[llvm] [lit] Fix some issues from --per-test-coverage (PR #65242)
Joel E. Denny via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 11 08:02:05 PDT 2023
jdenny-ornl wrote:
> I noticed that the python formatting check failed so I looked into that, the problem is the file `llvm/utils/lit/tests/per-test-coverage.py`
Thanks for pointing that out. I noticed it too but made the bad assumption that something was wrong with the pre-commit check itself, and I ran out of time to investigate. This is the diagnostic I saw after a traceback:
```
black.parsing.InvalidInput: Cannot parse: 12:13: t-cfg.py ({{[^)]*}})
```
I didn't see `llvm/utils/lit/tests/per-test-coverage.py` mentioned anywhere. Did I miss it?
In contrast, when I run black on the command line on my local system, I see:
```
$ black llvm/utils/lit/tests/per-test-coverage.py
error: cannot format llvm/utils/lit/tests/per-test-coverage.py: Cannot parse: 12:13: t-cfg.py ({{[^)]*}})
Oh no! 💥 💔 💥
1 file failed to reformat.
```
Is there any way to get the pre-commit check to mention the name of the problematic file?
(By the way, I have noticed that many python files were formatted with >80 columns, probably the black default of 88. That doesn't fit in the 80-column editor/terminal windows I use for most of LLVM. Is there any good reason not to follow [the LLVM coding standard for source code width](https://llvm.org/docs/CodingStandards.html#source-code-width) in python files? I see no exception there for python, or did I misunderstand something? Black just needs a `-l80` command-line option.)
>
> this file is not valid python, neither `black` or `python` can parse it. Since that's the case, can we change the extension on it so that we don't have to create a lot of exceptions in our CI and pre-commit hook?
Lit's tests use the `.py` suffix. Most just contain comments that contain lit/FileCheck directives. Some contain actual python code called by RUN lines. The above test contains garbage outside of a comment, so the test works fine, but it's invalid as python. I'll push a commit to this PR to fix it. Thanks again for pointing me in the right direction.
https://github.com/llvm/llvm-project/pull/65242
More information about the llvm-commits
mailing list