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

Mocha tests fail on modern Node versions #633

Closed
lishaduck opened this issue Jul 8, 2024 · 4 comments · Fixed by #635
Closed

Mocha tests fail on modern Node versions #633

lishaduck opened this issue Jul 8, 2024 · 4 comments · Fixed by #635

Comments

@lishaduck
Copy link

lishaduck commented Jul 8, 2024

It looks like the repo could use some cleanup :)
The solution is the same as jfmengels/node-elm-review#118 (comment)
The root cause is that JSON.parse returns better errors on modern node versions, so my node 20 has a different error than the repo's node 14(? 16?) (12 is the engine, so maybe that?)
What node do we want for tests? Should we patch the tests to ~equal? Patch the output? How do we want to go about fixing this?
With elm-review, we locked our node, but then we can't test for compatibility. I'd probably patch the tests to say it should equal one of different messages.

Found testing mpizenberg/elm-solve-deps-wasm#3.

@lishaduck lishaduck changed the title Mocha tests fail on modern node Mocha tests fail on modern Node versions Jul 8, 2024
@lishaduck
Copy link
Author

P.S.: If you're comfortable, @lydell, I think elm-review's test suite is enough to merge mpizenberg/elm-solve-deps-wasm#3 given that these don't seem to work, but if you'd prefer to test those changes first, I can work on this. I might either way 🤷‍♂️

@lydell
Copy link
Collaborator

lydell commented Jul 8, 2024

I already ran the tests myself, and had one test failing before and after installing your changes to elm-solve-deps-wasm due to the changed JSON.parse error messages. So that should be good to go.

To make the test pass again across Node.js versions, I’d just do the simplest thing that solves the problem in the test. Not sure what without looking closer to it.

@lishaduck
Copy link
Author

lishaduck commented Jul 8, 2024

I already ran the tests myself, and had one test failing before and after installing your changes to elm-solve-deps-wasm due to the changed JSON.parse error messages. So that should be good to go.

Cool, thanks!

To make the test pass again across Node.js versions, I’d just do the simplest thing that solves the problem in the test. Not sure what without looking closer to it.

I'll send in a PR to patch the comparison sometime.

@lishaduck
Copy link
Author

Opened #634

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