AbstractXpathTestSupport does not support generating multiple xpath suppressions

[Vyom-Yadav]

AbstractXpathTestSupport does not support generating multiple xpath suppressions

[nrmancuso]

@TanayMorakhia awesome investigation, thanks for this. @Vyom-Yadav @TanayMorakhia can you come to with some high level design of how we would add “true” support for multiple violations and outline the work to be done? I am still not sold on the value of this task, and would like to get an idea of the lift required.

@Vyom-Yadav please extend issue description with results from investigation at #14435 (comment)

[Vyom-Yadav]

cc @romani @nrmancuso

[nrmancuso]

If we haven't required this capability yet, I would think it is safe to assume that we won't in the future. Can we just throw some descriptive exception if an XPath test input has more than one violation?

[Vyom-Yadav]

Can we just throw some descriptive exception if an XPath test input has more than one violation?

The method accepts an array as an input argument, technically, we should change that, too. Changing all tests would be more work than adding multiple processing logic. Hence, I think we can support multiple violations.

[nrmancuso]

The method accepts an array as an input argument, technically, we should change that, too

For a test method, I don't see any issue with just throwing an exception here.

This makes failure obvious to contributors, and avoids hundreds of updates or a lot of work for low ROI.

[nrmancuso]

Looks like we do support this, somehow:

[ERROR] Errors: 
[ERROR]   XpathRegressionAvoidNestedBlocksTest.testSwitchAllowInSwitchCaseFalse:113->AbstractXpathTestSupport.runVerifications:204 » IllegalArgument Expected violations should contain only one element. Multiple violations are not supported.
[ERROR]   XpathRegressionAvoidNestedBlocksTest.testSwitchAllowInSwitchCaseTrue:144->AbstractXpathTestSupport.runVerifications:204 » IllegalArgument Expected violations should contain only one element. Multiple violations are not supported.
[ERROR]   XpathRegressionUnnecessarySemicolonInTryWithResourcesTest.testDefault:60->AbstractXpathTestSupport.runVerifications:204 » IllegalArgument Expected violations should contain only one element. Multiple violations are not supported.

From 05da9c0

[TanayMorakhia]

Looks like we do support this, somehow:

Not Exactly, they are working because all the violation have the same Xpath Queries.

checkstyle/src/it/java/org/checkstyle/suppressionxpathfilter/XpathRegressionAvoidNestedBlocksTest.java

Lines 93 to 100 in c3104f1

	final String[] expectedViolation = {
	"9:21: " + getCheckMessage(AvoidNestedBlocksCheck.class,
	AvoidNestedBlocksCheck.MSG_KEY_BLOCK_NESTED),
	"16:13: " + getCheckMessage(AvoidNestedBlocksCheck.class,
	AvoidNestedBlocksCheck.MSG_KEY_BLOCK_NESTED),
	"20:21: " + getCheckMessage(AvoidNestedBlocksCheck.class,
	AvoidNestedBlocksCheck.MSG_KEY_BLOCK_NESTED),
	};

For this the expath queries are:

>>>java -jar checkstyle-10.13.0-all.jar -s 9:21 src\it\resources\org\checkstyle\suppressionxpathfilter\avoidnestedblocks\SuppressionXpathRegressionAvoidNestedBlocksSwitch1.java
/COMPILATION_UNIT/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionAvoidNestedBlocksSwitch1']]/OBJBLOCK/METHOD_DEF[./IDENT[@text='s']]/SLIST/LITERAL_SWITCH/CASE_GROUP/SLIST
/COMPILATION_UNIT/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionAvoidNestedBlocksSwitch1']]/OBJBLOCK/METHOD_DEF[./IDENT[@text='s']]/SLIST/LITERAL_SWITCH/CASE_GROUP/SLIST/SLIST


>>>java -jar checkstyle-10.13.0-all.jar -s 16:13 src\it\resources\org\checkstyle\suppressionxpathfilter\avoidnestedblocks\SuppressionXpathRegressionAvoidNestedBlocksSwitch1.java
/COMPILATION_UNIT/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionAvoidNestedBlocksSwitch1']]/OBJBLOCK/METHOD_DEF[./IDENT[@text='s']]/SLIST/LITERAL_SWITCH/CASE_GROUP/SLIST/SLIST


>>>java -jar checkstyle-10.13.0-all.jar -s 20:21 src\it\resources\org\checkstyle\suppressionxpathfilter\avoidnestedblocks\SuppressionXpathRegressionAvoidNestedBlocksSwitch1.java
/COMPILATION_UNIT/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionAvoidNestedBlocksSwitch1']]/OBJBLOCK/METHOD_DEF[./IDENT[@text='s']]/SLIST/LITERAL_SWITCH/CASE_GROUP/SLIST
/COMPILATION_UNIT/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionAvoidNestedBlocksSwitch1']]/OBJBLOCK/METHOD_DEF[./IDENT[@text='s']]/SLIST/LITERAL_SWITCH/CASE_GROUP/SLIST/SLIST

Confirmed using diff-checker xD

Xpath queries is different for line 16:13 but it is subset of other queries that's the reason it is working.

[Vyom-Yadav]

can you come to with some high level design of how we would add “true” support for multiple violations and outline the work to be done?

Changes should be pretty straightforward:

final ViolationPosition position =                      
        extractLineAndColumnNumber(expectedViolations); 
final List<String> generatedXpathQueries =              
        generateXpathQueries(fileToProcess, position);  

Instead of doing it for just extracting one violation position, extract all violation positions. Generate XPath queries for all violations.

I think this will do the trick, rest every method is already configured to handle multiple violations. @TanayMorakhia @nrmancuso wdyt?

[Vyom-Yadav]

We generate XPath queries for all violations. They may be different/same, but that won't matter; we can have repeated elements in the expected array. The generated arr would be populated by generating XPath suppressions for every violation individually.

I think having same suppressions in expected arr wont be a problem, I'll be okay with that. Alternatively we may use a map, but that'll require updating every test!

[TanayMorakhia]

Instead of doing it for just extracting one violation position, extract all violation positions. Generate XPath queries for all violations.

Yes, this will work.

I think having same suppressions in expected arr wont be a problem, I'll be okay with that. Alternatively we may use a map, but that'll require updating every test!

I also thing going with arr is the best option as changing all the tests for map would be huge task.

I am still not sold on the value of this task, and would like to get an idea of the lift required.

It would add value by having better test cases and maybe number of test cases can be reduced per check if we can have multiple violations in the same file.
According to me it's implementation would require changes in the following classes and methods:

• AbstractXpathTestSupport.extractLineAndColumnNumber() for multiple violationPositions
• AbstractXpathTestSupport.generateXpathQueries() for multiple XpathQueries
• XpathQueryGenerator - to have arr of lines and columns because if not we would have to create it's multiple instance (number of violations) in generateXpathQueries(), which would be inefficient.
• XpathQueryGenerator.generate() - to return multiple queries.

@nrmancuso @Vyom-Yadav Kindly suggest if this is correct approach or not.

[TanayMorakhia]

@nrmancuso @Vyom-Yadav Ping

[Vyom-Yadav]

@TanayMorakhia I would do it a bit differently:

AbstractXpathTestSupport.extractLineAndColumnNumber() for multiple violationPositions

Instead of modifying the method for multiple violations, use a loop and and get pos for all violations.

AbstractXpathTestSupport.generateXpathQueries() for multiple XpathQueries

No, don't do that, if we generate suppressions for multiple queries together we might have overlapping suppressions, which would confuse future contributors. Use a loop and get suppressions for every violation.

Using loop for methods that accept a single input is the easiest way to go and works well. That's what I'd suggest.

[TanayMorakhia]

Makes sense.
what else needs to be done for this issue to be approved ?

[romani]

I don't see a reason of this issue.

@nrmancuso , do you see a reason?
If not , please close issue.

[Vyom-Yadav]

I don't see a reason of this issue.

The method signature is misleading, it accepts String[] expectedViolations but in reality, you can not pass more than one violation. Either change this to a string or handle multiple violations, this can throw off new contributors.

[nrmancuso]

I don't see a reason of this issue.

@nrmancuso , do you see a reason? If not , please close issue.

I can see how this issue could cause confusion (to anyone) expecting it to correctly handle multiple xpath expressions. I can't see putting any more effort into this than throwing an exception if more than one is provided though, and that is all I am currently prepared to approve this for.

Like I stated above, this exception would solve the problem, the change would be easy to make, and the PR easy to review. I don't see much value in extending this to actually support multiple expressions if we haven't needed them yet.

[romani]

Ok, let's just throw exception, let contributor quickly know that it not allowed.

[TanayMorakhia]

I am on it.

[TanayMorakhia]

Should I fix the tests with multiple queries in the same PR ?

[TanayMorakhia]

@romani @nrmancuso

[Vyom-Yadav]

@TanayMorakhia Are there any tests that use multiple queries? Here is the solution we are going for:

Ok, let's just throw exception, let contributor quickly know that it not allowed.

If there are tests that use multiple queries (they would be wrong in the first place), and yes, you can remove those usages in the same PR.

[TanayMorakhia]

Looks like we do support this, somehow:

[ERROR] Errors: 
[ERROR]   XpathRegressionAvoidNestedBlocksTest.testSwitchAllowInSwitchCaseFalse:113->AbstractXpathTestSupport.runVerifications:204 » IllegalArgument Expected violations should contain only one element. Multiple violations are not supported.
[ERROR]   XpathRegressionAvoidNestedBlocksTest.testSwitchAllowInSwitchCaseTrue:144->AbstractXpathTestSupport.runVerifications:204 » IllegalArgument Expected violations should contain only one element. Multiple violations are not supported.
[ERROR]   XpathRegressionUnnecessarySemicolonInTryWithResourcesTest.testDefault:60->AbstractXpathTestSupport.runVerifications:204 » IllegalArgument Expected violations should contain only one element. Multiple violations are not supported.

From 05da9c0

These tests have multiple queries... working on it.

[nrmancuso]

Closed via #14577