-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Try to re-factor the integration-boot.js
file (and related code)
#12730
Labels
Comments
Can I work on it? |
Sure, feel free to work on this. |
Hey, Can I work on this? |
This issue is unlikely to be a good beginner bug, if you're not already at least somewhat familiar with the PDF.js code-base and its test-suites. |
timvandermeij
added a commit
to timvandermeij/pdf.js
that referenced
this issue
Jun 23, 2024
Currently errors in `afterAll` are logged, but don't fail the tests. This could cause new errors during test teardown to go by unnoticed. Moreover, the integration test use a different reporting mechanism which also handled errors differently (this is extra reason to do mozilla#12730). This patch fixes the issues by consistently handling errors in `suiteStarted` and `suiteDone` in both reporting mechanisms. Fixes mozilla#18319.
timvandermeij
added a commit
to timvandermeij/pdf.js
that referenced
this issue
Jun 23, 2024
Currently errors in `afterAll` are logged, but don't fail the tests. This could cause new errors during test teardown to go by unnoticed. Moreover, the integration test use a different reporting mechanism which also handled errors differently (this is extra reason to do mozilla#12730). This patch fixes the issues by consistently handling errors in `suiteStarted` and `suiteDone` in both reporting mechanisms. Fixes mozilla#18319.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
For consistency/maintainability reasons, it would probably be helpful if the new
integration-boot.js
(and related code) would look more like the existing code used with the unit/font-tests. To that end, I'd suggest the following (not an exhaustive list):test/integration/jasmine-boot.js
and possibly adding a newtest/integration/integration_test.html
file as well as part of these changes.integration-boot.js
, and any related code intest/test.js
, file to follow the same general pattern as used with e.g. https://github.com/mozilla/pdf.js/blob/master/test/unit/jasmine-boot.js and https://github.com/mozilla/pdf.js/blob/master/test/unit/unit_test.html respectively https://github.com/mozilla/pdf.js/blob/master/test/font/jasmine-boot.js and https://github.com/mozilla/pdf.js/blob/master/test/font/font_test.htmlpdf.js/test/integration-boot.js
Lines 35 to 40 in 00b4f86
pdf.js/test/test.js
Lines 797 to 798 in 00b4f86
The text was updated successfully, but these errors were encountered: