Issue #10745: Add inline config support for tests with multiple modules #10758
Conversation
src/test/java/com/puppycrawl/tools/checkstyle/AbstractModuleTestSupport.java
Show resolved
Hide resolved
shashwatj07
force-pushed
the
107451
branch
2 times, most recently
from
146ab2a
to
53e053e
Compare
shashwatj07
force-pushed
the
107451
branch
2 times, most recently
from
93ebe8f
to
db34644
Compare
| @@ -389,59 +378,27 @@ protected final void verifyWithLimitedResources(Configuration aConfig, | |||
| throws Exception { | |||
| // We return null here, which gives us a result to make an assertion about | |||
| final Void result = TestUtil.getResultWithLimitedResources(() -> { | |||
| verifyWithInlineConfigParser(aConfig, fileName, expected); | |||
| verifyWithInlineConfigParser(fileName, expected); | |||
There was a problem hiding this comment.
@shashwatj07 @romani How are we verifying a configuration run without providing it said configuration?
When I remove the method parameter since aConfig is no longer used, I see ParenPad and SingleSpace tests creating a configuration for no reason. It doesn't make sense.
Edit: I think I get a sense now what this commit is doing. It's getting the configuration from the input file.
If this is not breaking tests, then we should remove the configuration from the method parameter as it no longer serves a purpose. We are expecting the configuration to be in the input file.
There was a problem hiding this comment.
I feel more comfortable, for now, its not breaking tests by the new issue I created.
This can be a simple parameter removal in a new PR.
There was a problem hiding this comment.
I get your point. I wonder why CI didn't complain of unused parameter. I'll remove it.
I will send PR to throw proper exception.
There was a problem hiding this comment.
I am not sure about PMD or TeamCity, but Checkstyle itself has no check to catch something like this. I only found it because my Eclipse configuration is set to produce more errors on things like. We can't enable it in CI because it produces false exceptions that can't be suppressed.
There was a problem hiding this comment.
I'm suspecting PMD. Although, I get your point. Thank you.
There was a problem hiding this comment.
@pbludov You probably know PMD best. PMD has UnusedFormalParameter. Do we have it enabled and should it have caught this?
https://www.ing.iac.es//~docs/external/java/pmd/rules/unusedcode.html
Resolves #10745
Resolves #10171