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.

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

Rename annotation_layer_spec.js to annotation_spec.js to better describe what is actually tested, and simplify the FileAttachmentAnnotation unit-test to avoid having to use the entire API in the test #7951

Merged
merged 2 commits into from Jan 12, 2017

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Jan 12, 2017

Every other unit-test in annotation_spec.js is already only testing the annotation code. Hence it seems unnecessarily convoluted to make use of the API here, when we can (fairly) simply provide the necessary data explicitly as in all the other annotation unit-test.

Edit: Note how doing this will also allow us to run the annotation unit-tests on Travis!
Compare the run with this PR: https://travis-ci.org/mozilla/pdf.js/builds/191392213#L369; with a previous run: https://travis-ci.org/mozilla/pdf.js/builds/191349139#L350.

Copy link
Contributor

@yurydelendik yurydelendik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename 'annotation_layer_spec.js' to 'annotation_spec.js' (and use it in the 'clitools.json')? Also can you remove PDFJS/displayGlobal? I don't think we have tests for display/annotation_layer.js here

…nit-tests only cover `src/core/annotation.js` functionality
…use the entire API in the test

Every other unit-test in `annotation_spec.js` is already only testing the annotation code. Hence it seems unnecessarily convoluted to make use of the API here, when we can (fairly) simply provide the necessary data explicitly as in all the other annotation unit-test.
@Snuffleupagus Snuffleupagus changed the title Simplify the FileAttachmentAnnotation unit-test to avoid having to use the entire API in the test Rename annotation_layer_spec.js to annotation_spec.js to better describe what is actually tested, and simplify the FileAttachmentAnnotation unit-test to avoid having to use the entire API in the test Jan 12, 2017
@Snuffleupagus
Copy link
Collaborator Author

Can you rename 'annotation_layer_spec.js' to 'annotation_spec.js' (and use it in the 'clitools.json')? Also can you remove PDFJS/displayGlobal? I don't think we have tests for display/annotation_layer.js here

Good call on the file name, and the fact that displayGlobal is now unused; I've addressed both points now.
Thanks for the review!

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/10dea6062d55a2a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/65460847de6ff5f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/65460847de6ff5f/output.txt

Total script time: 2.00 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/10dea6062d55a2a/output.txt

Total script time: 2.70 mins

  • Unit Tests: Passed

@yurydelendik yurydelendik merged commit c0d7029 into mozilla:master Jan 12, 2017
@yurydelendik
Copy link
Contributor

Thank you for the patch.

@Snuffleupagus Snuffleupagus deleted the FileAttachmentAnnotation-simplified-unittest branch January 12, 2017 18:53
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…tation-simplified-unittest

Rename `annotation_layer_spec.js` to `annotation_spec.js` to better describe what is actually tested, and simplify the `FileAttachmentAnnotation` unit-test to avoid having to use the entire API in the test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants