[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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




Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp:9
+// RUN: mkdir %t
+// RUN: echo '[{"directory":"%/S","command":"clang++ -c %s","file":"%s"}]' \
+// RUN:  > %t/compile_commands.json

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D74299#2338823 , @aaron.ballman 
wrote:

> In D74299#2338772 , @AlexanderLanin 
> wrote:
>
>> Could someone commit this as I cannot? Thanks a lot!
>> Alexander Lanin 
>>
>> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D74299#2338772 , @AlexanderLanin 
wrote:

> Could someone commit this as I cannot? Thanks a lot!
> Alexander Lanin 
>
> Thanks for review @aaron.ballman!

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



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin marked an inline comment as done.
AlexanderLanin added a comment.

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin 

Thanks for review @aaron.ballman!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 298566.
AlexanderLanin added a comment.

Readd removed 'print help without crashing' test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp


Index: 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
@@ -0,0 +1,33 @@
+// 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 
+int main()
+{
+  std::vector vec;
+  std::vector::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
+}
+
Index: 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp
@@ -0,0 +1,33 @@
+// 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;
+}
Index: clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp
+++ /dev/null
@@ -1,18 +0,0 @@
-// 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;
-}


Index: clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
@@ -0,0 +1,33 @@
+// under test:
+// - parsing and using compile_commands
+// - export fixes to yaml file
+
+// use %t as directory 

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin planned changes to this revision.
AlexanderLanin added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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




Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp:1
-// RUN: %run_clang_tidy --help
 // RUN: rm -rf %t

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

*ping*


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

*ping*


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

*ping*


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74299/new/

https://reviews.llvm.org/D74299



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits