-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat: update test file format to support aggregate functions #736
feat: update test file format to support aggregate functions #736
Conversation
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.
Reminder: Eventually we want to stop checking these in if possible.
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.
Once the grammar changes are stable, the checkins to auto-generated files will stop (I think it will be sooner than later, we have to add existing BFT tests in new format here).
We have a pre-commit check on test coverage so we need this file in this repo.
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.
If the files are generated by the author then there's a chance that the grammar changed but the parser files did not. By running the generation as part of the coverage check we can ensure that the files are matching.
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.
Agree with @EpsilonPrime . We can have the coverage test include:
- generate the parser
- run the tests
So we should ultimately land with no need to check in generated files. (Their only purpose is to complete the tests.)
One other ticket we should think about: add CI checks that new functions have test coverage and test coverage shouldn't go down.
I will address this in another PR.
I have added a check here. This should catch if a new function is added without testcases. Once we have more tests checked in, we can improve the check to report functions with no coverage or less coverage. |
No description provided.