Skip to content

Make node --harmony bin/cake test pass on Node 9 #5012

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

Merged

Conversation

rdeforest
Copy link
Contributor

Make classMaker() explicitly return an object

Without this change, node --harmony bin/cake test the test "classes with value'd constructors" fails because classMaker returns a number.

Make classMaker() explicitly return an object
@rdeforest
Copy link
Contributor Author

Should I also add

  • node --harmony ./bin/cake test

to appveyor.yml?

@GeoffreyBooth
Copy link
Collaborator

We should add Node 9 for both AppVeyor and Travis. And harmony as one of the test commands we run. If harmony tests fail in Node < 9, then we should have harmony only run for Node 9 (because why would someone run an old version of Node in harmony mode?).

@GeoffreyBooth
Copy link
Collaborator

@rdeforest
Copy link
Contributor Author

rdeforest commented Mar 17, 2018

Seeing my Cakefile patch in use in #4880 was the best birthday present I got yesterday. :)

Back on topic, harmony tests are currently passing for me on v6 and v8. I agree in principle that there's little to no customer value in making bleeding-edge features tests run and/or pass on non-bleeding-edge runtimes, but until they actually fail I propose that they could serve as a kind of canary-in-a-coal-mine for other surprises. They might reveal obscure changes in older runtimes, dependencies or invalid assumptions in our feature tests.

@GeoffreyBooth GeoffreyBooth merged commit ce66a49 into jashkenas:master Mar 18, 2018
@rdeforest rdeforest deleted the return-object-from-classmaker branch March 18, 2018 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants