Please stop reporting clang-format errors in test directories

[AaronBallman]

The precommit CI pipeline reports clang-format errors, which is incredibly useful. However, it reports them in test files, which renders the functionality effectively useless because we've never enforced coding style in tests and so those false positive reports typically swamp the true positive issues.

Further, we now have people trying to add clang-format tags to the test files to work around this, which adds additional burden on reviewers to mention that these comments are distracting and unnecessary because tests aren't required to be formatted. (We should probably document this better in the coding style guide, but this has been the status quo for ages because test files often exceed 80 col limits when verifying diagnostics, we almost never follow the capitalization and indentation schemes, etc.

Can the precommit CI pipeline please stop reporting clang-format issues in tests?

[llvmbot]

@llvm/issue-subscribers-infrastructure

[akyrtzi]

we've never enforced coding style in tests

It's even worse than that, some tests are specifically written in a way to test something, and that way can be incompatible with the clang-format output.

[AaronBallman]

This has been extremely distracting as a code reviewer. Not being able to trust precommit CI failures because they're spurious ~90% of the time is training reviewers to ignore failing precommit CI and is wasting code review time where authors are adding formatting tags to tests and reviewers are asking to remove the formatting tags. Can this please be addressed?

CC @ChristianKuehnel @metaflow in case they can help out here.

[martinboehme]

It should be possible to do this simply by adding a .clang-format file to clang/test with this:

{
    "DisableFormat": true,
    "SortIncludes": "Never",
}

I notice however that clang/test already contains a .clang-format file with this:

BasedOnStyle: LLVM
ColumnLimit: 0
AlwaysBreakTemplateDeclarations: No

Looking at the history, it looks as if the intent was to make clang-format "do something useful" on tests. However, the back and forth on the formatting options seems to indicate that this wasn't really useful.

I propose the contents of the file should be changed to turn off formatting completely. I may submit a patch today if I get round to it.

Edit: Having taken a look at the clang-format documentation, I believe just the DisableFormat line should be sufficient.

[martinboehme]

Patch submitted for review:

https://reviews.llvm.org/D128706

I'd like to make sure we have consensus that this is the right change to make, so I'd like multiple reviewers to weigh in. Do you have recommendations for who to add?

[AaronBallman]

Looking at the history, it looks as if the intent was to make clang-format "do something useful" on tests. However, the back and forth on the formatting options seems to indicate that this wasn't really useful.

Thank you for the sleuth work, I hadn't realized we already controlled this!

I'd like to make sure we have consensus that this is the right change to make, so I'd like multiple reviewers to weigh in. Do you have recommendations for who to add?

I mentioned this on the review, but because this is changing something that seems at least partially intentional, we should probably have an RFC on Discourse. Then the reviewers matter less, we're just checking that you did what you said you'd do in the RFC.

[martinboehme]

I mentioned this on the review, but because this is changing something that seems at least partially intentional, we should probably have an RFC on Discourse. Then the reviewers matter less, we're just checking that you did what you said you'd do in the RFC.

Good idea -- done:

https://discourse.llvm.org/t/rfc-disable-clang-format-in-the-clang-test-tree/63498