-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
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. |
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.
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.
I have looked into this, but unfortunately this is more difficult than I had hoped. The main issue is that the unit/font tests run in the browser itself whereas the integration tests run in Node.js and only control the browsers under test. This means that Jasmine also only runs once in Node.js instead of twice in each browser. This makes it very difficult to use the same general structure or reporting mechanism because those rely on being run inside the browser under test. One way in which I can imagine this working is if we run Puppeteer inside the individual browsers. This should be possible according to https://pptr.dev/guides/running-puppeteer-in-the-browser, but in quickly trying this I haven't gotten it to work yet. Even if this works though we'll have to find a solution for tests that currently rely on Node.js packages (first-party like I therefore think the path of making the browsers orchestrate the tests here isn't going to fly. What might work instead is to let Node.js remain the orchestrator, but find some way to parametrize the tests for Jasmine so that it can report on the individual browsers inside of each test running the logic per browser inside the test. This would basically mean moving the I think mainly having the browser name in the test output would be most helpful for e.g. debugging intermittents, but I don't yet see how we can easily achieve that. If there are ideas about this I'd love to hear about it. I did make PR #19254 for the first point mentioned here to at least somewhat improve consistency. |
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: