Prefer BugCheckerRefactoringTestHelper#doTest(TestMode.TEXT_MATCH) over alternatives

[rickie]

Problem

This is specific to the BugCheckerRefactoringTestHelper which is used in replacement tests for BugPatterns.
There are two TestModes, namely AST_MATCH and TEXT_MATCH. During multiple code reviews we noticed that using AST_MATCH can hide bugs. Therefore we always prefer TEXT_MATCH.

Besides that, we do not like the statically imported TEXT_MATCH, as it results in having a little less context. Therefore we want to rewrite all these occurrences as well, using Refaster.

Description of the proposed new feature

• Support a stylistic preference.
• Avoid a common gotcha, or potential problem.

We would like to rewrite the following code using Refaster:

refactoringTestHelper.addInputLines("A.java", "").addOutputLines("A.java", "").doTest();
refactoringTestHelper.addInputLines("B.java", "").addOutputLines("A.java", "").doTest(TestMode.AST_MATCH);
refactoringTestHelper.addInputLines("C.java", "").addOutputLines("A.java", "").doTest(TEXT_MATCH);
refactoringTestHelper.addInputLines("C.java", "").addOutputLines("A.java", "").doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);

to:

refactoringTestHelper.addInputLines("A.java", "").addOutputLines("A.java", "").doTest(TestMode.TEXT_MATCH);
refactoringTestHelper.addInputLines("B.java", "").addOutputLines("B.java", "").doTest(TestMode.TEXT_MATCH);
refactoringTestHelper.addInputLines("C.java", "").addOutputLines("C.java", "").doTest(TestMode.TEXT_MATCH);
refactoringTestHelper.addInputLines("C.java", "").addOutputLines("C.java", "").doTest(TestMode.TEXT_MATCH);

Considerations

The rewrite from TestMode.AST_MATCH to TestMode.TEXT_MATCH is not a behavior preserving one. It's important to make a note about that by using @Description on the Refaster rule.

[Stephan202]

The rewrite from TestMode.AST_MATCH to TestMode.TEXT_MATCH is not a behavior preserving one.

Likewise for .doTest().

[rickie]

Thanks for pointing out, forgot to mention that. The implementation of .doTest() delegates to .doTests(TestMode.AST_MATCH).

[rickie]

Tweaked the ticket to also cover the following rewrite:
BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH --> TestMode.TEXT_MATCH.

[mlrprananta]

Will pick this one up 🙂 Should this be a Refaster rule?

[Stephan202]

Nice @mlrprananta! I don't think this can be a Refaster rule. Rather, it sounds like a slight extension of what's being worked on in #450. WDYT @rickie @cernat-catalin?

[cernat-catalin]

Identification and qualifying the type definitely fall into that one!

We could extend the rule to also replace certain identifiers 🤔 .
I'm just thinking that we might make the rule a bit convoluted (i.e. we will have types which we only qualify, types which we only replace and types which we both qualify and replace; plus exemptions).

Maybe it would be better to have two separate rules and factor out some of the logic for identifier identification 🙃.