Lit test macro-to-enum.cpp does not work as expected.

[earnol]

When changing test lang-tools-extra/test/clang-tidy/checkers/modernize/macro-to-enum.cpp i have noticed adding:

diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-     to-enum/modernize-macro-to-enum.h
index f747be4761bb..eff79fe7e3c7 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum.h
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum.h
@@ -6,7 +6,7 @@
 #define GG_RED 0xFF0000
 #define GG_GREEN 0x00FF00
 #define GG_BLUE 0x0000FF
-// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enuuum
 // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_RED' defines an integral constant; prefer an enum instead
 // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_GREEN' defines an integral constant; prefer an enum instead
 // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_BLUE' defines an integral constant; prefer an enum instead

does not make test to fail as expected. Actually all CHECK-MESSAGES statement are completely ignored in the header files and test passes no matter what is written in those.
Observed behavior:
Test passes with completely wrong test statements.

Expected behavior:
Test fails if modified in unexpected way.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: None (earnol)

Details

When changing test lang-tools-extra/test/clang-tidy/checkers/modernize/macro-to-enum.cpp i have noticed adding: ``` diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro- to-enum/modernize-macro-to-enum.h index f747be4761bb..eff79fe7e3c7 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum.h +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum.h @@ -6,7 +6,7 @@ #define GG_RED 0xFF0000 #define GG_GREEN 0x00FF00 #define GG_BLUE 0x0000FF -// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum +// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enuuum // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_RED' defines an integral constant; prefer an enum instead // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_GREEN' defines an integral constant; prefer an enum instead // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GG_BLUE' defines an integral constant; prefer an enum instead ``` does not make test to fail as expected. Actually all CHECK-MESSAGES statement are completely ignored in the header files and test passes no matter what is written in those. **Observed behavior**: Test passes with completely wrong test statements.

Expected behavior:
Test fails if modified in unexpected way.

[LegalizeAdulthood]

The existing script that runs the tests doesn't validate header file contents; it's not really specific to this check.

At some point we want to extend the script to check header files too, but this hasn't been done yet.

[earnol]

Well. I noticed that as well. Because i tried to use this an example for my own lit test which was checkering warnings in the header file and encountered the fact test is not working. On the contrary the test clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp working as expected. The problem is the non-existent feature is used in this test and it does not sound right to me.

[zeyi2]

After some grepping [1] and verifying, I think currently these files are affected by the missing feature in the script:

• clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum3.h
• clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum2.h
• clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/macro-to-enum/modernize-macro-to-enum.h
• clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include/duplicate-include.h

[1] grep -lR "CHECK-MESSAGES\|CHECK-FIXES" clang-tools-extra/test/clang-tidy | grep -E "\.(h|hpp)$", although only part of the files are actually affected.

[vbvictor]

There is a hack to test fixes in headers involving manual diffing two files:

llvm-project/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-header.cpp

Lines 1 to 7 in cd806d7

	// RUN: rm -rf %t
	// RUN: mkdir %t
	// RUN: cp %S/Inputs/unnecessary-value-param/header.h %t/header.h
	// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t/temp -- -- -I %t
	// RUN: diff %t/header.h %S/Inputs/unnecessary-value-param/header-fixed.h
	#include "header.h"

[carlosgalvezp]

Duplicate:
#133515

[earnol]

I do not see any mentions of the clang-tools-extra/test/clang-tidy/checkers/modernize/macro-to-enum.cpp in the issue #133515. I agree the root cause for both issues is the same.

[LegalizeAdulthood]

It's the same problem; I originally wrote this check and am the one that added the annotations to the header files you've mentioned. As stated in the other bug, my intent was to address the shortcoming in the python script, but churn on the script kept me from rebasing my changes without extensive conflicts and so I needed to start over. I recently tried starting over with Copilot to help me out and made some progress, but it's not yet finished.

So, to sum up: it's not really a bug with this check, it's a bug in the script that runs the tests.

This is particularly true as the subject of this issue references the source file, not the header file. The source file is validated properly. It's the header file that isn't validated because the test script lacks the functionality.

[carlosgalvezp]

Should we then close this issue as duplicated?

[earnol]

I think it can be closed as duplicate with simultaneous addition of this case to #133515 as right now this failure is not mentioned in the issue 133515.

[zeyi2]

Closed as duplicate of #133515. Thanks for everyone involved.

Further discussion could move to there :)