Align TestMode import across test classes

[philleonard]

Small cleanup to align with other test classes in this package.

🧹 Whilst an ad-hoc cleanup when working on something else, I'm not sure it can be generalised (and likely one you have already considered so I'm interested to know your opinions!).

What factors come into play when deciding between an import and a static import for accessing static members of a class? Ones I consider that cannot be generalised (trivially):

• Trade-off between the class qualification providing extra descriptive value, and adding to verbosity. Requires understanding the descriptive value of a the qualifying class name in the context it is (statically or not) imported.
• Frequency as a multiplier. Within one class I assume a check would be trivial, across many classes of the same structure not trivial with EP. In this case, it is common to all(?) #replacement tests and therefore you could argue we could drop the qualifier. This leads onto another question: should we not provide some form of test abstraction (harness) for BugChecker tests? Might also be nice to look at a cleaner test data setup, or perhaps there is a reason to providing varargs String's in CompilationTestHelper#addSourceLines over reading from a .java source file?

Suggested commit message (not much to say about this change so probably too verbose/redundant):

Qualify non-static `TestMode` imports across `BugChecker` test classes (#630)

Prefer non-static imports and qualification of
`BugCheckerRefactoringTestHelper#TestMode` members across bug checker test
classes.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[rickie]

Heyy @philleonard , thanks for opening the PR! 🚀

First some context related to this specific issue and the question about generalization:

• We have a ticket that is related to this specific issue: Prefer BugCheckerRefactoringTestHelper#doTest(TestMode.TEXT_MATCH) over alternatives #389.
• @cernat-catalin opened a PR for a related check that is the exact opposite of the StaticImport check.

Nice thoughts, and yes you are right, it is not easy to generalize this and auto-detect candidates for static importing. For that reason we have a curated set of candidates for statically importing things.

We have the rule that we should not "lose" any context if we statically import something. This usually holds for imports that have
words (or parts) that are (partially) duplicate with the method. Additionally it should not be ambiguous. For example, if there are multiple places from which you can import the constant UTC, it is not directly clear from where you imported something, so we don't want to do it.

I have to go now so will get back to your question about the test abstraction later 😄.

[Stephan202]

Generally we try to combine PRs such as this one with a check that enforces the change, but as @rickie says we already have #450 for this. I'm fine with merging this PR as-is :)

I tweaked the suggested commit message :)

This leads onto another question: should we not provide some form of test abstraction (harness) for BugChecker tests? Might also be nice to look at a cleaner test data setup, or perhaps there is a reason to providing varargs String's in CompilationTestHelper#addSourceLines over reading from a .java source file?

Error Prone does support moving the test code to separate resource files, but in practice this makes test maintenance harder, and as such we see that new upstream Error Prone tests also prefer the current approach.

That said... yes, we can do better 😄. See #198, where we'll move to text blocks. That in turn unlocks IntelliJ IDEA language injection support :)

[philleonard]

where we'll move to text blocks. That in turn unlocks IntelliJ IDEA language injection support :)

Nice! Having played around with #198 this is already a much nicer experience for writing these test classes (which I have been doing in a scratch file for now). Copilot suggestions and IntelliJ autocomplete also "work" within language injections so that is nice. After a quick scan, GitHub and the IntelliLang both have somewhat "incomplete" syntax highlighting, but already much better than what we currently have.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication