Static Code Checks for Test Files #1257

schedutron · 2018-04-04T14:16:21Z

(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.

schedutron · 2018-04-04T14:18:57Z

@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?

tcm-marcel · 2018-04-04T14:34:00Z

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

schedutron · 2018-04-04T15:01:36Z

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.

coveralls · 2018-04-04T16:16:15Z

Coverage increased (+0.01%) to 77.566% when pulling 3d74f74 on schedutron:static into 5686479 on cmu-db:master.

tcm-marcel

Hey, I just took a quick look and found a few problems:

at the moment the script accidentally picks up forward declarations:

04-06-2018 13:18:59 [check_tests:217] INFO : Invalid class name in /home/marcel/dev/peloton/peloton_review/test/concurrency/testing_transaction_util.cpp
04-06-2018 13:18:59 [check_tests:219] INFO : Line 34: class ExecutorContext;
04-06-2018 13:18:59 [check_tests:219] INFO : Line 37: class InsertPlan;
04-06-2018 13:18:59 [check_tests:219] INFO : Line 38: class ProjectInfo;
04-06-2018 13:18:59 [check_tests:221] INFO : Test class names should end with 'Tests' suffix.

methods are not checked yet: TEST_F(..)

It looks like all other matches are correct, meaning that the rules are actually violated there. So once we agreed on the result, we will add one more commit that fixes the naming in the files.

schedutron · 2018-04-11T14:53:53Z

@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?

tcm-marcel · 2018-04-11T15:08:47Z

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 _util suffix?

Sounds good.

schedutron · 2018-04-11T15:11:43Z

Alright, but do we have to check after we extract the identifiers?

tcm-marcel · 2018-04-11T15:14:30Z

Ah sorry, I got you wrong. The naming conventions say, that every method (test case) should have the suffix Test.

schedutron · 2018-04-11T15:16:27Z

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?

tcm-marcel · 2018-04-11T15:20:29Z

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.

schedutron

I have couple of doubts.

schedutron · 2018-04-23T22:08:43Z

script/validators/source_validator.py

+                        class_status = False
+                    LOG.info("Line %s: %s", line_num, line.strip())
+
+            else:


Should there be a separate loop for this? That will prevent interleaved output.

I am ok with this, we hope not to have thousands of those errors.

schedutron · 2018-04-23T22:09:24Z

script/validators/source_validator.py

@@ -66,6 +66,8 @@
    "src/codegen/util/cc_hash_table.cpp"
 ]

+TEST_CASE_PATT = re.compile(r'TEST_F\(([a-zA-Z]+), ([a-zA-Z]+)\)')


Is this the right place to define the regex?

I think so.

tcm-marcel

Looks good!
Next step would be to fix all wrong test cases. In generel this should be easy, maybe we have to do some refactoring there.

tcm-marcel · 2018-04-24T15:36:27Z

script/validators/source_validator.py

@@ -224,6 +259,11 @@ def validate_file(file_path):
        if not validator(file_path):
            file_status = False

+    relative_path = file_path.replace(PELOTON_DIR, '')


One more thing: Can you use os.path.relpath here instead of string replace?

schedutron · 2018-04-27T04:30:56Z

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?

tcm-marcel · 2018-04-27T13:47:57Z

@schedutron No, we don't provide automated fixing. We expect people to fix their violations manually ;)

schedutron

I can see one obvious issue: there were few classes ending with Test rather than Tests. What happened during the fix is that they're now suffixed with TestTests. I'll fix these soon.

schedutron · 2018-04-28T04:12:20Z

test/binder/binder_test.cpp

@@ -36,7 +36,7 @@ using std::make_tuple;
 namespace peloton {
 namespace test {

-class BinderCorrectnessTest : public PelotonTest {
+class BinderCorrectnessTestTests : public PelotonTest {


This class should've been named BinderCorrectnessTests!

@schedutron Can you apply this change and rebase the PR? How many changes of the test files are still missing?

It's time that we merge it in and finish that part.

Sorry for getting this delayed. I'll apply the changes soon.

schedutron · 2018-05-10T06:17:49Z

test/binder/binder_test.cpp

@@ -36,7 +36,7 @@ using std::make_tuple;
 namespace peloton {
 namespace test {

-class BinderCorrectnessTest : public PelotonTest {
+class BinderCorrectnessTests : public PelotonTest {


@tcm-marcel At this stage, there are parent classes which still have the Test suffix instead of the Tests suffix. They are defined in .h files, and thus were not changed by the name-fixing script I ran. If I change them, I would also have to change their name in inheritance statements, e.g., here the statement will look like class BinderCorrectnessTests : public PelotonTests {. Should I do that?

@schedutron Yes.

schedutron · 2018-05-10T08:52:30Z

@tcm-marcel Have a look now :)

tcm-marcel · 2018-05-11T14:28:19Z

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.

schedutron · 2018-05-13T20:48:23Z

@tcm-marcel I was afraid that could happen. Will fix this soon. Thanks for the tips!

schedutron · 2018-05-19T13:34:25Z

@tcm-marcel The checks are passing now!

tcm-marcel · 2018-05-21T17:47:07Z

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.

schedutron · 2018-05-21T18:44:02Z

What exactly are the changes in, say, test/codegen/oa_hash_table_test.cpp?

tcm-marcel · 2018-05-21T18:54:13Z

I can't find it anymore... seems like I looked at a wrong version. I am sorry! Let me take another look.

tcm-marcel · 2018-05-21T22:51:02Z

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.

saatviks · 2018-05-24T00:51:58Z

@schedutron can you fix the merge conflicts so this can be merged in?

tli2 · 2018-06-05T15:49:26Z

What is the status of this PR right now? I am putting In Progress tag until merge conflicts are fixed.

schedutron · 2018-08-22T08:40:05Z

Lets finalize this!

tcm-marcel added the in progress label Apr 4, 2018

tcm-marcel changed the title ~~Static Code Checks for Test Files (in progress)~~ Static Code Checks for Test Files Apr 4, 2018

tcm-marcel self-requested a review April 4, 2018 22:26

tcm-marcel reviewed Apr 6, 2018

View reviewed changes

schedutron force-pushed the static branch from 619e848 to 77c97c1 Compare April 23, 2018 22:07

schedutron commented Apr 23, 2018

View reviewed changes

tcm-marcel reviewed Apr 24, 2018

View reviewed changes

schedutron commented Apr 28, 2018

View reviewed changes

schedutron commented May 10, 2018

View reviewed changes

schedutron force-pushed the static branch from 00607e8 to f502f2c Compare May 18, 2018 15:21

tcm-marcel force-pushed the static branch from a402c4e to 3d74f74 Compare May 21, 2018 22:49

schedutron added 4 commits May 21, 2018 18:51

Fix test naming

3b54ec1

lint

353e31d

Fix scattered discrepencies

b1cc1a4

Add test validation code

44cacfa

tcm-marcel force-pushed the static branch from 3d74f74 to 44cacfa Compare May 21, 2018 23:01

tcm-marcel added accepted and removed in progress labels May 21, 2018

tcm-marcel approved these changes May 22, 2018

View reviewed changes

tcm-marcel mentioned this pull request May 26, 2018

Support COPY for CSV files #1371

Merged

tli2 added the in progress label Jun 5, 2018

Static Code Checks for Test Files #1257

Are you sure you want to change the base?

Static Code Checks for Test Files #1257

Conversation

schedutron commented Apr 4, 2018 • edited by tcm-marcel Loading

schedutron commented Apr 4, 2018 • edited Loading

tcm-marcel commented Apr 4, 2018

schedutron commented Apr 4, 2018 • edited Loading

coveralls commented Apr 4, 2018 • edited Loading

tcm-marcel left a comment

Choose a reason for hiding this comment

schedutron commented Apr 11, 2018 • edited Loading

tcm-marcel commented Apr 11, 2018

schedutron commented Apr 11, 2018

tcm-marcel commented Apr 11, 2018

schedutron commented Apr 11, 2018

tcm-marcel commented Apr 11, 2018 • edited Loading

schedutron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcm-marcel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schedutron commented Apr 27, 2018 • edited Loading

tcm-marcel commented Apr 27, 2018

schedutron left a comment

Choose a reason for hiding this comment

schedutron Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schedutron commented May 10, 2018

tcm-marcel commented May 11, 2018 • edited Loading

schedutron commented May 13, 2018

schedutron commented May 19, 2018

tcm-marcel commented May 21, 2018 • edited Loading

schedutron commented May 21, 2018

tcm-marcel commented May 21, 2018

tcm-marcel commented May 21, 2018

saatviks commented May 24, 2018

tli2 commented Jun 5, 2018

schedutron commented Aug 22, 2018

schedutron commented Apr 4, 2018 •

edited by tcm-marcel

Loading

schedutron commented Apr 4, 2018 •

edited

Loading

schedutron commented Apr 4, 2018 •

edited

Loading

coveralls commented Apr 4, 2018 •

edited

Loading

schedutron commented Apr 11, 2018 •

edited

Loading

tcm-marcel commented Apr 11, 2018 •

edited

Loading

schedutron commented Apr 27, 2018 •

edited

Loading

schedutron Apr 28, 2018 •

edited

Loading

tcm-marcel commented May 11, 2018 •

edited

Loading

tcm-marcel commented May 21, 2018 •

edited

Loading