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

test older parsers don't throw non-syntax errors on new syntax #746

Conversation

iliabylich
Copy link
Collaborator

raising an error in the else branch makes 42 tests to fail (i.e. there are 42 breaking MRI changes that we are aware of)

@mbj PTAL, do you have anything to add?

@iliabylich iliabylich force-pushed the make-sure-that-tests-dont-throw-non-parser-errors-on-older-parsers branch from df2cddb to b1879af Compare September 26, 2020 11:22
@mbj
Copy link
Collaborator

mbj commented Sep 26, 2020

PTAL, do you have anything to add?

This prevents regressions like the ones I reported in #742. I think it qualifies to the idea we had in the discussions around that issue.

Thanks.

@iliabylich iliabylich changed the title make sure that tests don't throw non-syntax errors on older versions of parser test older parsers don't throw non-syntax errors on new syntax Sep 26, 2020
@iliabylich iliabylich merged commit 4ac776e into whitequark:master Sep 26, 2020
@iliabylich iliabylich deleted the make-sure-that-tests-dont-throw-non-parser-errors-on-older-parsers branch September 26, 2020 14:51
@marcandre
Copy link
Contributor

Cool stuff. I imagine that a test that the AST produced is different could be added (in the no error case)?

@mbj
Copy link
Collaborator

mbj commented Sep 26, 2020

Cool stuff. I imagine that a test that the AST produced is different could be added (in the no error case)?

Yes, that makes sense. May require some whitelisting for new syntax added that is just sugar for existing AST. But likely a good default.

OT: I really like these "static-property-tests". Where you take a corpus of test inputs and try to find as many correctness signals as possible.

Another OT: @iliabylich I've started to re-use parsers test suite in unparser (found tons of issues in unparser) and I'm currently resorting to this hack: https://github.com/mbj/unparser/pull/148/files#diff-6e44895a7a81a0ca94584e0d4d9ebc1f. Basically I implement a "fake" minitest to capture the examples.

I wounder if you'd support changing most of the tests to be defined in a data structure that is walked by minitest. This way use cases like unparser (and likely other tools that need to be exposed to syntax edge cases) can require that data without having to resort to "drive by capturing" as I do over at that unparser branch.

If you like the idea I'd be happy to start a PR on that.

Note we could still generate the exactly same minitest surface. Just the minitest test_foo a-like methods would be generated from that data structure.

@iliabylich
Copy link
Collaborator Author

Cool stuff. I imagine that a test that the AST produced is different could be added

Agree, maybe we'll find that some SINCE_X_Y rules are set incorrectly.

Another OT: @iliabylich I've started to re-use parsers test suite in unparser (found tons of issues in unparser) and I'm currently resorting to this hack: mbj/unparser#148 (files). Basically I implement a "fake" minitest to capture the examples.

haha, I did the same multiple times (https://github.com/sorbet/sorbet/blob/master/tools/scripts/import_whitequark.rb for example)

I wounder if you'd support changing most of the tests to be defined in a data structure that is walked by minitest

Could you provide an example of how it might look like? I'm not sure if I fully understand what you want to be simplified, existing test suite looks quite simple to me for exporting

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.

None yet

3 participants