-
Notifications
You must be signed in to change notification settings - Fork 119
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
Integrate Test262 #83
Conversation
Retrieve the Test262 project from its canonical location on GitHub.com. Use the `test262-stream` module to create tests in accordance with that project's interpretation guidelines. Use the `test-interpreter` project to maintain a expectations file of tests which Esprima is currently expected to fail.
@lee-dohm @maxbrunsfeld This patch can no longer be merged to do conflicting changes on the master branch. I can see about resolving them, but are you still interested in integrating Test262? |
@lee-dohm @maxbrunsfeld this patch was meant as a follow-up to our discussion in the language-javascript project. Is there someone else I should be talking to about it? |
Hey @jugglinmike. I'm following up on this very old issue and pull request. A couple of us working on semgrep are now members of the tree-sitter-javascript and tree-sitter-typescript projects, as well as grammars for various other languages. We are particularly interested in having reliable parsers and fixing errors before our customers encounter them. I think we would greatly benefit from testing tree-sitter-javascript against Test262. Let me see if I can answer your original questions:
Given that
I don't know about javascript scripts. How are they different from any other javascript file? Is it just that first line is a magic number like rules: {
program: $ => seq(
optional($.hash_bang_line),
repeat($.statement)
), An optional hashbang line is supported so maybe this answers the question about scripts. I'd be in favor of moving forward with this PR. Our shared concerned is maintenance. I don't see an issue here since this is not an intrusive change. Even if running the Test262 suite for some reason becomes a problem, we can always disable it. Let's see what others have to say. I'll follow up in a week if there's no reaction. Cc: my colleague at r2c @aryx, and @maxbrunsfeld |
Hi there, @mjambon. "Script" and "Module" are two parsing goals for JavaScript These two goals cannot be differentiated by inspecting the program source code Some Test262 tests are written specifically for the Module parsing goal. When export default 0; And it might have another test with the same source, but that's expected to In order to pass both of those tests, you'll nee to make You can read Test262's |
How long does it take to parse test262? I remember this was a really huge repository with a huge number of files (like > 10 000 if I remember correctly). |
test262 could be part of a parsing testsuite, like the one we have in semgrep with a list of top 20 github repo for each language. |
Thanks for the explanation, @jugglinmike.
Tree-sitter is focused on text-editor syntax highlighting, and semgrep uses it for searching for patterns in code. In general, we're ok with invalid code that parses without an error. A problem would be valid code that doesn't parse or valid code that's misinterpreted, producing an incorrect parse tree. That's why I think the tree-sitter-javascript parser, with its single entrypoint, should be able to parse both scripts and modules. If a user wants to make sure there are no Earlier, you showed this result:
So if we run our grammar on Test262, we'll pay attention only to lines TP and FN. We'll try minimize FN (false negatives) and maximize TP (true positives). In other words, we don't care whether invalid programs are reported as valid or invalid. For IDE support, it's actually best to parse as much as possible of an invalid program. Tree-sitter has an error recovery mechanism which allows producing a partial parse tree, with special ERROR nodes for the portions that it can't parse. This mechanism is also useful to tolerate errors due to the grammar being incomplete. |
@@ -0,0 +1,172 @@ | |||
#!/usr/bin/env node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we can accept this PR, this source file should include a description of:
- what it's intended for (including when should it run, who should run it),
- what it does,
- how it's achieving it,
- how it should be maintained and evolve (including how to manage the file(s) listing expected failures)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most important is to ensure:
- a PR is blocked when there's a regression or a progression;
- print what to do in case of a regression or unexpected failure: how to locate the source snippet, how to mark it as an expected failure if needed;
- print what to do if a test that used to fail now passes: how to remove it from the list of expected failures
Retrieve the Test262 project from its canonical location on GitHub.com.
Use the
test262-stream
module to create tests in accordance with thatproject's interpretation guidelines. Use the
test-interpreter
projectto maintain a expectations file of tests which Esprima is currently
expected to fail.
This patch introduces two new npm dependencies during development:
test262-stream
- A Node.js readable stream for traversing the Test262 test suite (this creates the test code based on the contents of the Test262 directory)results-interpreter
- A Node.js writable stream for interpreting streaming test results (this implements the code necessary to interpret the results in terms of an expectations file)These projects were created to reduce duplication (both in code and in maintenance effort) between Test262 consumers. They're both used by JSHint, Babel, and Esprima. Acorn uses just
test262-stream
, along with many folks testing JavaScript runtimes throughtest262-harness
.As noted in the process output, the expectations file can be automatically updated by specifying the
--update-expectations
command-line flag. It's also safe to manually insert comments, so you folks can keep notes about the failures in-line.Here's the summary of the results:
This is meant as a starting point. In addition to the feedback you folks have for me, there are a few details which I'd like to work out: