[clang-tidy] extend tests of run-clang-tidy

[AlexanderLanin]

Summary

• new test: parsing and using compile_commands
• new test: export fixes to yaml file
• old test extended with CHECK-MESSAGES in order to ensure that they "fail as intended"

[AlexanderLanin]

I tested this in WSL1 with ninja check-clang-tools.
Is there anything else I can do (besides setting up a second OS)?

[AlexanderLanin]

*ping*

[vmiklos]

This looks reasonable to me, though I haven't ever approved a review, so I would leave that to others set as reviewers.

[AlexanderLanin]

*ping*

[AlexanderLanin]

*ping*

Just new tests, easy to review :-)

In case this one is ok, please feel free to commit it as I cannot :-)
Alexander Lanin <[e-mail]>

[vmiklos]

@ilya-biryukov @lebedev.ri does this look OK to you?

[AlexanderLanin]

*ping*

[AlexanderLanin]

*ping*

[AlexanderLanin]

*ping*

[aaron.ballman]

Generally looks good to me, but I had a question about test coverage.

clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp

1

Is there a reason to remove this test coverage? I believe this is testing that we can access the help without crashing.

[AlexanderLanin]

The reason was that I thought that line doesn't do anything.
Of course "not crashing" is a valid expectation. If nothing else, then "not crashing" should indeed be tested.
I'll try to improve this ancient diff.

[AlexanderLanin]

Readd removed 'print help without crashing' test

[aaron.ballman]

LGTM!

[AlexanderLanin]

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <[e-mail]>

Thanks for review @aaron.ballman!

[aaron.ballman]

In D74299#2338772, @AlexanderLanin wrote:

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <[e-mail]>

Thanks for review @aaron.ballman!

Happy to do so! I've commit on your behalf in 627c01bee0deb353b3e3e90c1b8d0b6d73464466

[aaron.ballman]

In D74299#2338823, @aaron.ballman wrote:

In D74299#2338772, @AlexanderLanin wrote:

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <[e-mail]>

Thanks for review @aaron.ballman!

Happy to do so! I've commit on your behalf in 627c01bee0deb353b3e3e90c1b8d0b6d73464466

Unfortunately, I had to revert in b91a236ee1c3e9fa068df058164385732cb46bba because of failing build bots. Would you mind taking a look?

[AlexanderLanin]

I cannot reproduce the problem and there is just not enough info to go on.
Is there any way to get anything in addition? e.g. run the same test with -vv or add some additional print commands?
Maybe it's some access rights problem, or different version of some non obvious dependency or something like that.
Maybe it's as trivial as having to use double quotes for -checks='-*,bugprone-sizeof-container,modernize-use-auto'.
It seems run-clang-tidy exits with code 2, but there is no way it would do that. So I'm totally lost.
http://lab.llvm.org:8011/#/builders/109/builds/690/steps/6/logs/FAIL__Clang_Tools__run-clang-tidy_export-diagnosti

[aaron.ballman]

In D74299#2339994, @AlexanderLanin wrote:

I cannot reproduce the problem and there is just not enough info to go on.

Ugh, I hate when that happens. :-(

Is there any way to get anything in addition? e.g. run the same test with -vv or add some additional print commands?

We could speculatively commit with additional commands if that'd help you with debugging. We'd be purposefully breaking the build for a few minutes which is never ideal, but if we coordinate properly, it shouldn't be too disruptive.

Maybe it's some access rights problem, or different version of some non obvious dependency or something like that.
Maybe it's as trivial as having to use double quotes for -checks='-*,bugprone-sizeof-container,modernize-use-auto'.
It seems run-clang-tidy exits with code 2, but there is no way it would do that. So I'm totally lost.
http://lab.llvm.org:8011/#/builders/109/builds/690/steps/6/logs/FAIL__Clang_Tools__run-clang-tidy_export-diagnosti

Could it be that some other part of the test is what's generating that result code (like, could it be the result from python of running lit)?

clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp

9

The %/S here looks a bit suspicious to me, was that supposed to be %S with the / somewhere else?

clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp

-// RUN: %run_clang_tidy --help
-// RUN: rm -rf %t
-// RUN: mkdir %t
-// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c %/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\/\\\\/g' > %t/compile_commands.json
-// RUN: echo "Checks: '-*,modernize-use-auto'" > %t/.clang-tidy
-// RUN: echo "WarningsAsErrors: '*'" >> %t/.clang-tidy
-// RUN: echo "CheckOptions:" >> %t/.clang-tidy
-// RUN: echo "  - key:             modernize-use-auto.MinTypeNameLength" >> %t/.clang-tidy
-// RUN: echo "    value:           '0'" >> %t/.clang-tidy
-// RUN: cp "%s" "%t/test.cpp"
-// RUN: cd "%t"
-// RUN: not %run_clang_tidy "test.cpp"
-
-int main()
-{
-  int* x = new int();
-  delete x;
-}

clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp

+// under test:
+// - .clang-tidy read from file & treat warnings as errors
+// - return code of run-clang-tidy on those errors
+
+// First make sure clang-tidy is executable and can print help without crashing:
+// RUN: %run_clang_tidy --help
+
+// use %t as directory instead of file:
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// add this file to %t, add compile_commands for it and .clang-tidy config:
+// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c %/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\/\\\\/g' > %t/compile_commands.json
+// RUN: echo "Checks: '-*,modernize-use-auto'" > %t/.clang-tidy
+// RUN: echo "WarningsAsErrors: '*'" >> %t/.clang-tidy
+// RUN: echo "CheckOptions:" >> %t/.clang-tidy
+// RUN: echo "  - key:             modernize-use-auto.MinTypeNameLength" >> %t/.clang-tidy
+// RUN: echo "    value:           '0'" >> %t/.clang-tidy
+// RUN: cp "%s" "%t/test.cpp"
+
+// execute and check:
+// RUN: cd "%t"
+// RUN: not %run_clang_tidy "test.cpp" > %t/msg.txt 2>&1
+// RUN: FileCheck -input-file=%t/msg.txt -check-prefix=CHECK-MESSAGES %s \
+// RUN:           -implicit-check-not='{{warning|error|note}}:'
+
+int main()
+{
+  int* x = new int();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: {{.+}} [modernize-use-auto,-warnings-as-errors]
+
+  delete x;
+}

clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp

+// under test:
+// - parsing and using compile_commands
+// - export fixes to yaml file
+
+// use %t as directory instead of file,
+// because "compile_commands.json" must have exactly that name:
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo '[{"directory":"%/S","command":"clang++ -c %s","file":"%s"}]' \
+// RUN:      > %t/compile_commands.json
+
+// execute and check:
+// RUN: cd "%t"
+// RUN: %run_clang_tidy -checks='-*,bugprone-sizeof-container,modernize-use-auto' \
+// RUN:                 -p="%/t" -export-fixes=%t/fixes.yaml > %t/msg.txt 2>&1
+// RUN: FileCheck -input-file=%t/msg.txt -check-prefix=CHECK-MESSAGES %s \
+// RUN:           -implicit-check-not='{{warning|error|note}}:'
+// RUN: FileCheck -input-file=%t/fixes.yaml -check-prefix=CHECK-YAML %s
+
+#include <vector>
+int main()
+{
+  std::vector<int> vec;
+  std::vector<int>::iterator iter = vec.begin();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when declaring iterators
+  // CHECK-YAML: modernize-use-auto
+  
+  return sizeof(vec);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: sizeof() doesn't return the size of the container; did you mean .size()? [bugprone-sizeof-container]
+  // CHECK-YAML: bugprone-sizeof-container
+  // After https://reviews.llvm.org/D72730 --> CHECK-YAML-NOT: bugprone-sizeof-container
+}
+