This repository has been archived by the owner. It is now read-only.

Integrate Test262 #654

Merged
merged 11 commits into from Aug 7, 2017

Conversation

Projects
None yet
7 participants
@jugglinmike
Contributor

jugglinmike commented Jul 29, 2017

Introduce a GNU Make target for retrieving TC-39's Test262 suite and
validating parsing of the files it contains. Interpret each file as a
parser test in accordance with that project's INTERPRETING.md
document. Allow for the specification of allowed failures via a
"whitelist" file so that the test suite may help prevent regressions in
this project in situations where this project has known bugs. Initialize
the "whitelist" file with a listing of all tests that are currently
failing. Extend the continuous integration environment's configuration
to automatically run these tests.

Q A
Bug fix? no
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? yes
Fixed tickets gh-633
License MIT

Hey @hzoo!

Here's my first attempt at integrating Test262. There's a lot of new code here,
so I'm prepared to make a log of changes according to review feedback.

One thing I would prefer to do is split the run_test262_utils.js file into
multiple files, possibly within a dedicated directory. There's not much
precedent for that within the scripts directory, so I thought I'd hold off on
that until I heard back from you.

I'd also like to point out that the code within the scripts directory is not
currently being linted by the npm "lint" command. I manually satisfied the
linter anyway, with one exception: trailing commas. (I'm developing in Node.js
current LTS release where trailing commas are not supported.) Is there anything
you would like to change about this?

Integrate Test262
Introduce a GNU Make target for retrieving TC-39's Test262 suite and
validating parsing of the files it contains. Interpret each file as a
parser test in accordance with that project's `INTERPRETING.md`
document. Allow for the specification of allowed failures via a
"whitelist" file so that the test suite may help prevent regressions in
this project in situations where this project has known bugs. Initialize
the "whitelist" file with a listing of all tests that are currently
failing. Extend the continuous integration environment's configuration
to automatically run these tests.

@jugglinmike jugglinmike referenced this pull request Jul 29, 2017

Closed

Incorporating Test262 #633

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Jul 30, 2017

Member

Travis reported this error:

$ if [ "$JOB" = "test262-test" ]; then make test-test262; fi
node scripts/run_test262.js
{ Error: ENOENT: no such file or directory, stat '/home/travis/build/babel/babylon/build/test262/test'
    at Error (native)
  errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: '/home/travis/build/babel/babylon/build/test262/test' }

Is it intended?

Member

nicolo-ribaudo commented Jul 30, 2017

Travis reported this error:

$ if [ "$JOB" = "test262-test" ]; then make test-test262; fi
node scripts/run_test262.js
{ Error: ENOENT: no such file or directory, stat '/home/travis/build/babel/babylon/build/test262/test'
    at Error (native)
  errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: '/home/travis/build/babel/babylon/build/test262/test' }

Is it intended?

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Jul 30, 2017

Member

I see there are a lot of failing tests which babylon actually supports via plugins, for example language/expressions/async-generator/args-trailing-comma-multiple.js.
I think there should be a way to specify which plugins enable for a given file/folder.
Maybe we should use the features flag?

Member

nicolo-ribaudo commented Jul 30, 2017

I see there are a lot of failing tests which babylon actually supports via plugins, for example language/expressions/async-generator/args-trailing-comma-multiple.js.
I think there should be a way to specify which plugins enable for a given file/folder.
Maybe we should use the features flag?

jugglinmike added some commits Jul 30, 2017

@jugglinmike

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike Jul 30, 2017

Contributor

I think there should be a way to specify which plugins enable for a given
file/folder. Maybe we should use the features flag?

I'd be open to try that, but at some point, I think we should consider
introducing a YAML parser. The ad-hoc regular expressions I've written seem
"close enough" for the current use case, but using RegExps to interpret dynamic
values like that is much more error prone. I've been trying to avoid adding any
dependencies for this patch, so I'm wondering how the project maintainers would
feel about a new "devDependency" on a YAML parser.

Contributor

jugglinmike commented Jul 30, 2017

I think there should be a way to specify which plugins enable for a given
file/folder. Maybe we should use the features flag?

I'd be open to try that, but at some point, I think we should consider
introducing a YAML parser. The ad-hoc regular expressions I've written seem
"close enough" for the current use case, but using RegExps to interpret dynamic
values like that is much more error prone. I've been trying to avoid adding any
dependencies for this patch, so I'm wondering how the project maintainers would
feel about a new "devDependency" on a YAML parser.

@jugglinmike

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike Jul 30, 2017

Contributor

Alternatively, we could unconditionally enable all plugins for all tests. This is certainly less precision, but since Test262 generally doesn't include any "future hostile" tests, it may be an acceptable compromise.

Contributor

jugglinmike commented Jul 30, 2017

Alternatively, we could unconditionally enable all plugins for all tests. This is certainly less precision, but since Test262 generally doesn't include any "future hostile" tests, it may be an acceptable compromise.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 1, 2017

Member

We just did something like this for our printer tests (generator) babel/babel#6018 with explicit flags. We could do unconditional for all tests though for now, if someone runs the stage 0 preset, it would be the same thing and none of the plugins should be incompatible.

Member

hzoo commented Aug 1, 2017

We just did something like this for our printer tests (generator) babel/babel#6018 with explicit flags. We could do unconditional for all tests though for now, if someone runs the stage 0 preset, it would be the same thing and none of the plugins should be incompatible.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 1, 2017

Member

is there already a script/way to remove the whitelist tests automatically if things are fixed later?

Member

hzoo commented Aug 1, 2017

is there already a script/way to remove the whitelist tests automatically if things are fixed later?

Show outdated Hide outdated scripts/run_test262.js
Show outdated Hide outdated scripts/run_test262.js
badnews.push(desc);
badnewsDetails.push(desc + ":");
badnewsDetails.push(
...tests.map(function(test) {

This comment has been minimized.

@Kovensky

Kovensky Aug 2, 2017

Member

This also breaks in Node 4 😄

@Kovensky

Kovensky Aug 2, 2017

Member

This also breaks in Node 4 😄

Show outdated Hide outdated scripts/run_test262_utils.js
Show outdated Hide outdated scripts/run_test262_utils.js
return line.replace(/#.*$/, "").trim();
})
.filter(function(line) {
return line.length > 0;

This comment has been minimized.

@Kovensky

Kovensky Aug 2, 2017

Member

.filter(Boolean) would work too but I guess this is clearer

@Kovensky

Kovensky Aug 2, 2017

Member

.filter(Boolean) would work too but I guess this is clearer

@jugglinmike

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike Aug 5, 2017

Contributor

is there already a script/way to remove the whitelist tests automatically if
things are fixed later?

There was not, but that's a good idea. Maintaining the white list in JSHint has
been a pain point for me. I've updated the patch to include a command-line
flag and dedicated Makefile target.

So I think that's everything--this should be ready for another round of review.

Contributor

jugglinmike commented Aug 5, 2017

is there already a script/way to remove the whitelist tests automatically if
things are fixed later?

There was not, but that's a good idea. Maintaining the white list in JSHint has
been a pain point for me. I've updated the patch to include a command-line
flag and dedicated Makefile target.

So I think that's everything--this should be ready for another round of review.

@jugglinmike

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike Aug 6, 2017

Contributor

I've updated the job to run in the latest release of Node.js, and I've taken @Kovensky's advice regarding the "util.promisify" module. My thinking is that many contributors likely prefer to develop in the LTS release. Even though they may not all care to run this particular test suite, the module's dependency tree is light enough to be justify the convenience.

The babel-test job is failing. Is there something we should do about that in this patch?

Contributor

jugglinmike commented Aug 6, 2017

I've updated the job to run in the latest release of Node.js, and I've taken @Kovensky's advice regarding the "util.promisify" module. My thinking is that many contributors likely prefer to develop in the LTS release. Even though they may not all care to run this particular test suite, the module's dependency tree is light enough to be justify the convenience.

The babel-test job is failing. Is there something we should do about that in this patch?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 6, 2017

Member

There was not, but that's a good idea.

Like it would basically run the whitelist tests and if they pass correctly instead of erroring/not erroring it could tell you to remove them (or it is done automatically), similar to updating a snapshot/fixture

Member

hzoo commented Aug 6, 2017

There was not, but that's a good idea.

Like it would basically run the whitelist tests and if they pass correctly instead of erroring/not erroring it could tell you to remove them (or it is done automatically), similar to updating a snapshot/fixture

@hzoo

hzoo approved these changes Aug 6, 2017

lgtm, thanks for this work! Will help us a lot for regressions in the future! And hopefully get us started on the path for test262 in the transforms as well actually; ideally we can further more make compat-table 2.0 with test262

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 6, 2017

Member

oops I just merged babel/babel#6056, which means we need to update yarn at least?

either way unrelated to your pr

Member

hzoo commented Aug 6, 2017

oops I just merged babel/babel#6056, which means we need to update yarn at least?

either way unrelated to your pr

@jugglinmike

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike Aug 6, 2017

Contributor

Whoops; before you updated the comment, I thought you were saying I should update the yarn lock file. I guess wasn't necessary, after all (I'm not familiar with that tool). Should I revert this latest commit?

Contributor

jugglinmike commented Aug 6, 2017

Whoops; before you updated the comment, I thought you were saying I should update the yarn lock file. I guess wasn't necessary, after all (I'm not familiar with that tool). Should I revert this latest commit?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 6, 2017

Member

I updated Babel to use a new feature in yarn; I think we have to add https://github.com/babel/babel/pull/6056/files#diff-354f30a63fb0907d4ad57269548329e3L15 since the yarn version it's using now is older. Not sure it's going to fix but it's an issue due to us targeting master. Either way it would of broke in someone else's PR

Member

hzoo commented Aug 6, 2017

I updated Babel to use a new feature in yarn; I think we have to add https://github.com/babel/babel/pull/6056/files#diff-354f30a63fb0907d4ad57269548329e3L15 since the yarn version it's using now is older. Not sure it's going to fix but it's an issue due to us targeting master. Either way it would of broke in someone else's PR

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 6, 2017

Member

Yeah we don't need that commit, I can try this locally and push a change

Member

hzoo commented Aug 6, 2017

Yeah we don't need that commit, I can try this locally and push a change

@danez

danez approved these changes Aug 6, 2017

Nice work. It's crazy how much tests are not working with babylon.

@danez

This comment has been minimized.

Show comment
Hide comment
@danez

danez Aug 6, 2017

Member

I just tested this also locally and it doesn't work and stops with EMFILE: too many open files. Seems reading all the tests async without limit is not a good idea, but works on travis for some reason.

Member

danez commented Aug 6, 2017

I just tested this also locally and it doesn't work and stops with EMFILE: too many open files. Seems reading all the tests async without limit is not a good idea, but works on travis for some reason.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Aug 6, 2017

Member

We could use graceful-fs, which handles EMFILE errors automatically.

Member

nicolo-ribaudo commented Aug 6, 2017

We could use graceful-fs, which handles EMFILE errors automatically.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 6, 2017

Member

Yeah graceful-fs sounds good

Member

hzoo commented Aug 6, 2017

Yeah graceful-fs sounds good

existentialism added some commits Aug 6, 2017

@existentialism

existentialism approved these changes Aug 6, 2017 edited

Awesome @jugglinmike!

Note, I bumped our yarn version to support workspaces and switched to graceful-fs.

After this lands, we can drop the rest-parameter test262 test fixtures I added a while back

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 7, 2017

Member

Thanks for starting this @jugglinmike!

Member

hzoo commented Aug 7, 2017

Thanks for starting this @jugglinmike!

@hzoo hzoo merged commit 0466504 into babel:master Aug 7, 2017

2 of 3 checks passed

codecov/project 95.13% (-0.85%) compared to fb6d049
Details
codecov/patch Coverage not affected when comparing fb6d049...908fa7b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
bootstrap-test262: clean
mkdir ./build
git clone https://github.com/tc39/test262.git ./build/test262

This comment has been minimized.

@hzoo

hzoo Aug 7, 2017

Member

could we do a depth=x here as well?

@hzoo

hzoo Aug 7, 2017

Member

could we do a depth=x here as well?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Aug 7, 2017

Member

screen shot 2017-08-06 at 9 25 41 pm

Member

hzoo commented Aug 7, 2017

screen shot 2017-08-06 at 9 25 41 pm

@jugglinmike

This comment has been minimized.

Show comment
Hide comment
@jugglinmike

jugglinmike Aug 7, 2017

Contributor

It's been my pleasure :)

Contributor

jugglinmike commented Aug 7, 2017

It's been my pleasure :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.