Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor the test driver #6021

Merged
merged 5 commits into from
May 26, 2015
Merged

Refactor the test driver #6021

merged 5 commits into from
May 26, 2015

Conversation

timvandermeij
Copy link
Contributor

Some time ago I attempted to make the test suite work with the SVG back-end. I then found that the current testing code is really unmaintainable at some points and figured that it had to be corrected before we have a chance of adding new functionality (such as a back-end or WebGL switch).

This patch refactors the test driver by making it more class-like and removing quite some complexity from hacks and unneeded functions. Not only does this make the code much more readable, it also allows us to extend the code in a cleaner way because we got rid of the window.load dependency, innerHTML usages and many callbacks. Not all callbacks have been removed because the remaining ones are not too invasive and this patch can be seen as a major first step in cleaning up the test driver.

@Snuffleupagus Would you be up for reviewing this? It is not as daunting as it might look at first sight because a lot of it is just moving code around. The other changes are largely the same as the other refactoring PRs we have done. Reviewing with ?w=1 helps a bit, especially for the third commit.

@timvandermeij timvandermeij force-pushed the test-driver branch 5 times, most recently from c349259 to 7aabc6f Compare May 15, 2015 21:06
@timvandermeij
Refactor test slave
58769fd 
- Improved names of elements
- Easier scrolling code
@timvandermeij
Clean up the test driver
d59660c 
- Remove a hack for Chrome on Windows because we do not run Chrome on the Windows bot anymore, so it added unneeded complexity.
- Merge nextTask and continueNextTask functions. nextTask did nothing more than calling cleanup. This change also allows us to remove the callback for that function.
- Remove unnecessary one-line functions.
@timvandermeij timvandermeij force-pushed the test-driver branch 3 times, most recently from 8e57db3 to 3a57590 Compare May 15, 2015 21:49
@Snuffleupagus
Copy link
Collaborator

At this point it's still anecdotal evidence, but it appears that with these patches applied (and rebased to tip) the runtime of browsertest increased with at least 1 minute for me.

Edit: The increased runtime actually seems closer to 100 seconds.

@timvandermeij timvandermeij force-pushed the test-driver branch 2 times, most recently from 6679e52 to 8ab0865 Compare May 18, 2015 21:00
@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/76225ff6161b217/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/d3fe3fedf26fbb4/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/76225ff6161b217/output.txt

Total script time: 17.86 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/d3fe3fedf26fbb4/output.txt

Total script time: 17.94 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/c5f99abeed927f2/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/53393edd2380163/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/c5f99abeed927f2/output.txt

Total script time: 18.05 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://107.22.172.223:8877/c5f99abeed927f2/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/53393edd2380163/output.txt

Total script time: 19.01 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://107.21.233.14:8877/53393edd2380163/reftest-analyzer.html#web=eq.log

@timvandermeij
Copy link
Contributor Author

The previous run was with one PDF file (more specifically: pdfkit_compress.pdf) changed to make sure that failures get detected. I have removed that commit now as it was only meant for a test.

@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/a9204e17f770a87/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/b598dc584868b17/output.txt

@timvandermeij
Refactor test driver to be more class-like
ae8748e 
- Extract NullTextLayerBuilder and SimpleTextLayerBuilder from the driver code and create classes for them.
- Move options to the constructor and pass in HTML elements instead of getting them in the driver.
- Remove unused masterMode variable.
- Minor method name updates.

The rest is only moving/indenting existing code and adding 'this'.
@timvandermeij
Improve driver code structure
07ec736 
- Improve variable names and move variables to the top of the method
- Use constants where possible
- Only print the delay text if there is an atual delay set
- Simplify continuation logic in _nextTask
- Do not pass anything to clearCanvas: we can get the context from this.canvas there.
- Remove innerHTML and replace it with textContent. Add a comment why insertAdjancentHTML is so important for performance and runtime.
- Merge _quit and _sendQuitRequest.
@timvandermeij
Implement an option to disable automatic scrolling
e02ab9f 
This adds a checkbox with which one can disable scrolling, for example to look back at output during testing. Note that the styles are inline because the test runner removes all <style> elements for each test.
@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/f31bc60cdb7db6a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/cfae379f3b8edf6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/f31bc60cdb7db6a/output.txt

Total script time: 18.13 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/cfae379f3b8edf6/output.txt

Total script time: 20.40 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@timvandermeij
Copy link
Contributor Author

@Snuffleupagus Thank you for the review! You made some good points and I have addressed all of them. I have also moved some changed from the fourth commit to the third commit, where they indeed belonged in the first place. I ran the tests locally and verified that the results are the same as before today's changes and I just ran the tests here too.

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/c3544db4cc5cc9c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/313a6e57a3d8696/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/c3544db4cc5cc9c/output.txt

Total script time: 2.99 mins

  • Font tests: FAILED
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://107.22.172.223:8877/c3544db4cc5cc9c/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/313a6e57a3d8696/output.txt

Total script time: 18.02 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@Snuffleupagus
Copy link
Collaborator

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/fee6e6c0cc40d5e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/fee6e6c0cc40d5e/output.txt

Total script time: 20.60 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@Snuffleupagus
Copy link
Collaborator

Let's check that makeref-ing still works correctly before merging.

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/8bcdd70446125f0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/2de26a95a875686/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/8bcdd70446125f0/output.txt

Total script time: 1.05 mins

  • Lint: Passed
  • Make references: FAILED

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/2de26a95a875686/output.txt

Total script time: 18.05 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@timvandermeij
Copy link
Contributor Author

/botio-windows makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/e2e40a8a5b37c05/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/e2e40a8a5b37c05/output.txt

Total script time: 18.41 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@Snuffleupagus
Copy link
Collaborator

Since everything still appears to work, both on the bots and locally, let's do this!
Thank you for the patch!

Snuffleupagus added a commit that referenced this pull request May 26, 2015
@Snuffleupagus
Merge pull request #6021 from timvandermeij/test-driver
39d2103 
Refactor the test driver
@Snuffleupagus Snuffleupagus merged commit 39d2103 into mozilla:master May 26, 2015
@timvandermeij timvandermeij deleted the test-driver branch May 26, 2015 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Snuffleupagus Snuffleupagus

Labels
other
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@timvandermeij @Snuffleupagus @pdfjsbot