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

Default to ecmaVersion 7 in tests #614

Closed
wants to merge 1 commit into from
Closed

Conversation

adrianheine
Copy link
Member

No description provided.

@marijnh
Copy link
Member

marijnh commented Nov 2, 2017

I'm not sure we want to have the tests automatically roll forward as the default version is incremented. I mean, I don't see a huge problem with that, but some tests might be asserting that something works in a given version, and we'd be implicitly changing their meaning.

It'd be nice if we could set the version of the tests per-file or even per-region, possibly using some crude global variable hack. That being said, the organization of the tests could use work in general (there could be a lot more files, organized by theme/construct), but I don't really expect anyone to take on that boring job in the near future.

So uh, I don't want to block this on that, but I'd like some kind of kludge for mass-setting a language version to be added.

@RReverser
Copy link
Member

I don't really see what this particular change gains us, while modifying lots of lines and so losing the clear blame/history on these test files.

@adrianheine
Copy link
Member Author

How about we use version ranges for test cases and test with all these versions? As for setting the version in a file, a simple wrapper around test should be enough for that.
@RReverser Are you referring to the file splitting or the version bumping?

@RReverser
Copy link
Member

Kinda to both. But that aside, I don't really see what criteria was used to split tests to test-es5.js - most of tests in tests.js are already intended for ES5 with very few exclusions, and I'd rather move these.

@adrianheine
Copy link
Member Author

Ok, let's take a step back. I find it counter-intuitive if the tests don't use the same default settings as the production code. It's probably not a big thing, but I can definitely imagine myself being confused by that.

Also, I think it makes sense to have tests provide a set of valid ecmaVersion values, in most cases a range. Right now the ecmaVersion value in tests seems quite arbitrary and rarely only the one value specified is possible. Maybe it's overkill to test with all possible ecmaVersion values, but when reading the tests it's difficult to see what an ecmaVersion setting actually means: Is this the only version in which this test is true, is it the first one or the last one, or just an arbitrary one?

@RReverser
Copy link
Member

Is this still relevant in face of our recent ecmaVersion changes?

@marijnh
Copy link
Member

marijnh commented Oct 1, 2020

I guess it's outdated quite a lot by now anyway, though it's not great that it was just left to sit here without further discussion. But yeah, let's close.

@marijnh marijnh closed this Oct 1, 2020
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 this pull request may close these issues.

3 participants