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

Fix pull request #12131 so it can be merged again #12152

Closed
timvandermeij opened this issue Aug 1, 2020 · 7 comments · Fixed by #12154
Closed

Fix pull request #12131 so it can be merged again #12152

timvandermeij opened this issue Aug 1, 2020 · 7 comments · Fixed by #12154

Comments

@timvandermeij
Copy link
Contributor

It looks like the only problem that required us to do a revert of the patch is the fact that the unit tests in #12131 don't pass locally. However, they don't even run at all on the bots, hence we thought this was safe to merge given that all tests passed.

@timvandermeij Investigate why the bots did not run the test so this won't happen again.
@jsg2021 Could you improve the unit test so that we can try merging the patch again? Note the comment on the PR about what fails. In particular, we need to make sure that the tests work locally too; you can do that by running gulp unittest to test in the browser and gulp unittestcli to test in Node.js. Both should pass in order for the patch to be merged again. Thanks!

@jsg2021
Copy link
Contributor

jsg2021 commented Aug 1, 2020

yep, will do.

shouldn't npm test run both those? I was using that to run and validate locally.

@timvandermeij
Copy link
Contributor Author

Ah, now I understand. npm test runs the unit tests in Node.js and it was made this way so that Travis CI has a direct entrypoint for running the tests. The gulp unittest command instead runs the unit tests in the browser instead. There is a difference because we cannot run the same tests in the browser and in Node.js due to platform differences.

@timvandermeij
Copy link
Contributor Author

In short, the npm commands should not be used for development. Only the gulp tasks are valid for that. See https://github.com/mozilla/pdf.js/wiki/Contributing for more information.

@timvandermeij
Copy link
Contributor Author

I did find the root cause for the fact that the test failure was not reported on the bots, so I'll make a patch for that so that such problems don't go unnoticed again.

@jsg2021
Copy link
Contributor

jsg2021 commented Aug 1, 2020

@timvandermeij I've fixed the tests and pushed the branch. Should I issue a new pull request?

@timvandermeij
Copy link
Contributor Author

Yes, indeed. We have just merged the fix for the test reporter so that such problems don't go unnoticed anymore in the future, so please rebase onto the current master branch and create a new PR. Thank you!

@timvandermeij
Copy link
Contributor Author

Closing since the test reporter issue is fixed and there is a new PR for this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants