tausbn
commented
1 year ago In general, I think it’d be better to just wait to commit new tests until you’re fixing the parser to handle those cases. Otherwise, the test suite is in a somewhat strange state in the interim, in which it’s asserting behavior that we don’t actually specifically want.
That's fair. I always think of tests as merely documenting the present behaviour, and ensuring that regressions aren't introduced. I guess my view may be coloured somewhat by the fact that (in the context of the CodeQL Python analysis), documenting the limits of what the code can do is important, and fixing deficiencies may not be possible (since static analysis tools will basically always be incomplete 😅).
For a parser, it's a somewhat different situation.
However, it also occurs to me that simply by virtue of opening this PR, the CI has already demonstrated that the test cases I added indeed cause syntax errors at the moment (modulo the one you point out, which was a mistake on my part). So there's not actually any need to merge this PR. Future reviewers can just compare the test here with the one in the PR that fixes the problem.
maxbrunsfeld
These test cases demonstrate a couple of deficiencies in the current grammar.
I have created fixes for all of these issues, but I figured it would ease the review process (and increase confidence in the fixes) to first commit the tests that demonstrate the issues, as the follow-up PRs will then make it very clear that the issues have indeed been fixed.