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

Tests are broken on NodeJS 12.16.0+ #938

Closed
breautek opened this issue Mar 31, 2020 · 2 comments · Fixed by #939
Closed

Tests are broken on NodeJS 12.16.0+ #938

breautek opened this issue Mar 31, 2020 · 2 comments · Fixed by #939

Comments

@breautek
Copy link
Contributor

Tests that try to rewire any module with a shebang declaration are broken starting NodeJS 12.16.0.

The issue is being tracked at jhnns/rewire#178
I've created a PR for rewire at jhnns/rewire#179

This ticket is for us to keep track if/when rewire is updated or maybe to discuss alternative solutions, depending how long it will take for rewire to update. Their last update was 20 days ago, so could be some hope available.

@timbru31
Copy link
Member

/cc @erisu - we've had this discussion yesterday on Slack.
The underlying bug is IMO nodejs/modules#464

@breautek
Copy link
Contributor Author

I acknowledge that the issue should be really fixed at the NodeJS level. The PR is just a quick workaround for the time being.

In jhnns/rewire#178 they reference nodejs/node@bcd27f7300 & nodejs/node@1c50714729

I don't think moving away from rewire is a realistic approach as our repos are quite heavily invested in rewire. Without rewire a lot of code I think will need to be rewritten and exposed in a way so that the unit tests can have access to attach spies and such. It would be considerable amount of work to do, especially to maintain the same level of unit testing that we have.

If it takes a long time to see a fix from either NodeJS, or have my PR merged into rewire, would using my fork be acceptable?

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

Successfully merging a pull request may close this issue.

2 participants