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

add parser tests #559

Closed
wants to merge 1 commit into from
Closed

add parser tests #559

wants to merge 1 commit into from

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Mar 25, 2016

This PR adds several thousand tests designed to stress parsers lacking a corresponding evaluation engine, drawn from the test suites of the following open source projects:

See parser-tests/README.md for the full description. Briefly, there are source texts which should pass, fail, and produce early errors, and passing source texts have an alternative more legible version.

Tests have been rendered in a relatively uniform way, where possible, using a public node.js package. Because these tests are of a different kind than most of test262, their format is correspondingly different: files are not given descriptive names, and contain no header. Copyright information is provided in bulk in licenses.md.

@jugglinmike
Copy link
Contributor

I'm reluctant to accept this. On the one hand, I love tests, and more tests
might catch more bugs. But there are a few other concerns that give me pause.

First, there's the issue of validation. As the official test suite for the
ECMAScript specification, correctness in Test262 is critical. I fully expect
that you've done the legwork here, but the maintainers of this project have to
heed Reagan's advice: trust, but verify. Reviewing all 5,562 tests introduced
by this pull request will be an arduous task. This isn't a deal-breaker, of
course, but I don't know if you will be able to find a maintainer who is able
to conduct the kind of review this pull request requires.

More fundamentally, though, I'm concerned about test coverage.

Quite a lot of effort goes in to organizing tests. This is crucial for the
long-term maintenance of the project. As new tests are added (either to address
mistaken omissions or the introduction of new language features), a clear
organization scheme guides contributors both in where to place new tests and
how many tests are required. In other words, explicit organization allows for
discovery of coverage.

Because the tests in this patch eschew the organization we are working to
maintain, they detract from our ability to assses coverage. If we later find a
missing test case, the extent of the hole will be very difficult to define.

Accepting this patch work would reflect an approach to project maintenance that
is fundamentally different than what we've been observing. Following it
consistently would mean accepting similar patches in the future. A contributor
might even submit the output of an ECMAScript fuzzer, and precedence would
favor accepting this. The project would balloon in size, and while disk space
limitations are relevant, the increased time-to-execute would most certainly
present challenges for consumers.

That hypothetical may seem abstract, so I'll point out that it applies even
within this changeset. As a composition of many projects' tests, it only took a
few moments to identify duplication:

$ grep ? parser-test/fail/*
parser-test/fail/435.script.js:() ? 42
parser-test/fail/769.script.js:for(let ? b : c in 0);
parser-test/fail/887.script.js:() ? 0

I think we have a responsibility to optimize the resources (in both space and
time) we require of implementers, which in this case means avoiding
duplication.

I have a few ideas for alternatives that leverage the work here:

  • Classify these tests according to the organization of the project. Doing this
    in a piecemeal fashion will lessen the burden for you and for your reviewer.
  • Publish these tests as a standalone test suite. This new project could be
    advertised as a sort of "kitchen sink" test repository, with all the
    expansiveness/disorganization that implies. Consumers could benefit from both
    Test262 and this new project, possibly choosing to run the latter less
    frequently, as their needs dictate. TC-39 may itself even be interested in
    maintaining such a project.

To be clear: I am not a member of TC-39. I'd really like to hear @goyakin and
@bterlson have to say about all this!

@bakkot
Copy link
Contributor Author

bakkot commented Mar 28, 2016

@jugglinmike, thanks for the feedback. I'm certainly interested in the conversation about the usefulness of these tests, how they should best be organized, and if they belong in test262 at all; that's part of the point of this PR.

I'm not sure how to organize these tests according to the existing schema, particularly the failing tests. For example, it's not obvious to me what section of the spec } would go under. Similarly, many of the passing tests have to do with interactions with different parts of the spec.

Also, duplication is less of an issue than you might expect: that's the purpose of normalization. Only passing (and early error) tests are normalized, hence the duplication you see above, but then, it's not obvious to me what it means for two distinct texts to be duplicate failing tests.

@bakkot
Copy link
Contributor Author

bakkot commented Apr 1, 2016

I believe resolution from the committee meeting was that these should go in a new repository under tc39, which would still be under the purview of the test262 project but not necessarily maintained in the same way.

I'm leaving this PR open pending advancement on that front.

@michaelficarra
Copy link
Member

@littledan Can you help create this repository and grant the appropriate parties write permission?

@michaelficarra
Copy link
Member

Ping @littledan @bterlson. This is blocked on getting a new repo under the tc39 organisation.

@bterlson
Copy link
Member

I guess I should read the notes, but in the meantime I can certainly create the repo if someone can tell me what to call it?

@michaelficarra
Copy link
Member

I'd say test262-parser-tests is good enough for now. Please give write permissions to me, @bakkot, and all of the maintainers of test262.

@littledan
Copy link
Member

Oh sorry for missing this thread. @bakkot 's description of the resolution matches my understanding. Thanks for creating the repo, @bterlson .

@littledan
Copy link
Member

Note that test262-parser-tests should not be interpreted as "owning" the parser tests for test262, just taking a different approach.

@leobalter
Copy link
Member

https://github.com/tc39/test262-parser-tests is there with the test262 group + @michaelficarra and @bakkot as admins.

💯

@michaelficarra
Copy link
Member

Parser tests are now published at https://github.com/tc39/test262-parser-tests. This can be closed.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 14, 2016

Closing as completed elsewhere. Sorry for the delay in getting this finalized.

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.

None yet

6 participants