peloton
peloton copied to clipboard
cmu-db
Static Code Checks for Test Files
(Implementing #1192)
I've added a check for test class name. Do I also have to create another paths variable via glob module, which contains all the test files' paths (which end with *_test.cpp)?
Also, I'll add other checks soon.
@tcm-marcel Should check_tests function be only called on staged test files? Also, what are the criteria for the gtest macro methods to be checked? Should each test case name end with the 'Test' suffix?
I would check in validate_file(file_path) [1] if the file matches /test/*/*.cpp and then run the check in addition to the other checks. This way every test file that the validator is called with gets checked. The pre-commit hook calls the validator with the stages files, to this happens automatically.
[1] https://github.com/schedutron/peloton/blob/dda18900b9c5efafbea1d4ada73b2903ecd4dc9a/script/validators/source_validator.py#L237
I think I should add a path regex for checking whether we have a test file instead of checking membership in the path set returned by glob.glob() for faster execution. Will do so soon. Or maybe a startswith()-endswith() call pair would also be fine - and simpler.
Coverage increased (+0.01%) to 77.566% when pulling 3d74f74139f3648778016dded552968107c7a28d on schedutron:static into 5686479c5f33087031e1b68b7832245c7886b712 on cmu-db:master.
@tcm-marcel How do I check the methods? They don't follow a prefix/suffix pattern.
Also, to resolve the issue you mentioned above, should I omit the files that end with a _util suffix?
How do I check the methods? They don't follow a prefix/suffix pattern.
You can use a regex to find these lines and extract the information. (see example: https://regexr.com/3nnro)
Also, to resolve the issue you mentioned above, should I omit the files that end with a
_utilsuffix?
Sounds good.
Ah sorry, I got you wrong. The naming conventions say, that every method (test case) should have the suffix Test.
Yeah, but a lot of test cases across different test files do not. Are we going to use the validator script to fix all these files?
Yeah, but a lot of test cases across different test files do not. Are we going to use the validator script to fix all these files?
Correct, that's the idea. Unfortunately we have a lot of code that doesn't meet our conventions, because it is older than these conventions or because people didn't care. The validator shall help to prevent this in the future.
As a first iteration, I will simply add the Test suffix to the incorrect test class name and the Tests suffix to the incorrect test method names.
Also, do I need to implement the correction code in the formatter script or simply fix the incorrect files in the Python interpreter and push them?
@schedutron No, we don't provide automated fixing. We expect people to fix their violations manually ;)
Hey, I fear you screwed up the code when resolving the merge conflicts in 2f28acc. This is also the reason the tests fail.
I think the easiest way to solve this is to
- make a temporary copy of your branch
git checkout -b temp - reset the old branch (the one with the PR) to upstream master
git branch -f ...(don't forget to fetch and rebase your master branch before) - cherry pick the commits you want to have (all except the resolve merge conflicts one and the last one*)
- force push the new version of this branch, the PR will update accordingly
*Please remove the changes where you format the headers. It's a good idea, but it should be a separate PR.
If you need help, let me know.
@tcm-marcel I was afraid that could happen. Will fix this soon. Thanks for the tips!
Hey, sorry for looking at this only now.
~1) something is still wrong from rebasing, there are several changes in the test files, for example in test/codegen/oa_hash_table_test.cpp~
~2) I think the two classes PelotonTest and PelotonCodeGenTest should not be changed. I just created a commit to revert this, but I now realized that you will have to touch all files again anyway.~
Let me try if I can fix this quickly.
What exactly are the changes in, say, test/codegen/oa_hash_table_test.cpp?
I can't find it anymore... seems like I looked at a wrong version. I am sorry! Let me take another look.
Ok sorry, I messed this up a little bit:
- I was wrong, I must have looked at the wrong diff
- I thought about this again and decided that we leave it that way
I will try to rebase your PR and make it ready to merge.
What is the status of this PR right now? I am putting In Progress tag until merge conflicts are fixed.
