Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

split it module test support between different configurations #6801

Merged
merged 1 commit into from Jun 10, 2019

Conversation

Projects
None yet
3 participants
@rnveach
Copy link
Member

commented Jun 4, 2019

This should fix the issue identified at #6718 (comment) .
When attempting to add an IT test for sun, the code falsely said that the check didn't appear in the config. I later found out it was searching the google config for the check I had only added to the sun config. The IT test support was only looking at the google config and never the sun config, even if the IT tests were for sun. This PR fixes that issue by adding a module support class for each area: google, sun, checkstyle.

I will re-amend the commit with the correct Pull #.

@rnveach rnveach changed the title Pull #0: split it module test support between different configurations split it module test support between different configurations Jun 4, 2019

@rnveach rnveach force-pushed the rnveach:it_configs branch from ec9ea4a to 52dcd2c Jun 4, 2019

rnveach added a commit to rnveach/checkstyle that referenced this pull request Jun 4, 2019

@rnveach rnveach requested a review from romani Jun 4, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

ok to merge.

@pbludov , please finalize review.

@romani

romani approved these changes Jun 10, 2019

throw new IllegalStateException("problem to get ID attribute from " + conf, ex);
}
})
.findFirst().orElseGet(null);

This comment has been minimized.

Copy link
@pbludov

pbludov Jun 10, 2019

Collaborator

Should it be something like

.findFirst().orElseThrow(
  new IllegalStateException("problem with module config"));

? This case should not be possible, but if it happens, it is better to give a tip to the engineer.

This comment has been minimized.

Copy link
@rnveach

rnveach Jun 10, 2019

Author Member

Done.

* @param moduleId module id.
* @return {@link Configuration} instance for the given module name.
*/
protected static Configuration getModuleConfig(String moduleName, String moduleId) {

This comment has been minimized.

Copy link
@pbludov

pbludov Jun 10, 2019

Collaborator

You copied this method twice. Now there is duplicate code in AbstractGoogleModuleTestSupport and AbstractSunModuleTestSupport.
Does it make sense to move this into a separate class, common to Google and Sun tests?
It seems that only XML_NAME can be left from Abstract{Google,Sun}ModuleTestSupport.

This comment has been minimized.

Copy link
@rnveach

rnveach Jun 10, 2019

Author Member

This is not the only method copied between google and sun. The only difference between them is the config location, everything else is a copy/paste.
It is also not the only copy in the entire test structure. AbstractItModuleTestSupport and AbstractModuleTestSupport are basically copies but the main difference between them is the warning system required in the input files.

This comment has been minimized.

Copy link
@rnveach

rnveach Jun 10, 2019

Author Member

I can't split them apart because this method is static and the fields are static final and all these methods use these fields. This is why they were duplicated in the first place.

@@ -228,7 +195,7 @@ protected static DefaultConfiguration createRootConfig(Configuration config) {
* Expected messages are represented by the array of strings, warning line numbers are
* represented by the array of integers.
* This implementation uses overloaded
* {@link AbstractModuleTestSupport#verify(Checker, File[], String, String[], Integer...)}
* {@link AbstractGoogleModuleTestSupport#verify(Checker, File[], String, String[], Integer...)}

This comment has been minimized.

Copy link
@pbludov

pbludov Jun 10, 2019

Collaborator

AbstractGoogleModuleTestSupport => AbstractItModuleTestSupport

This comment has been minimized.

Copy link
@rnveach

rnveach Jun 10, 2019

Author Member

Done.

@rnveach rnveach force-pushed the rnveach:it_configs branch from 52dcd2c to eff55bf Jun 10, 2019

@pbludov
Copy link
Collaborator

left a comment

I'm OK to merge.

@rnveach rnveach added this to the 8.22 milestone Jun 10, 2019

@rnveach rnveach merged commit c7ffe16 into checkstyle:master Jun 10, 2019

16 checks passed

IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
Shippable Run 9758 status is SUCCESS.
Details
ci/circleci: build-caches Your tests passed on CircleCI!
Details
ci/circleci: pitest1 Your tests passed on CircleCI!
Details
ci/circleci: pitest2 Your tests passed on CircleCI!
Details
ci/circleci: pitest3 Your tests passed on CircleCI!
Details
ci/circleci: pitest4 Your tests passed on CircleCI!
Details
ci/circleci: pitest5 Your tests passed on CircleCI!
Details
ci/circleci: pitest6 Your tests passed on CircleCI!
Details
ci/circleci: pitest8 Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 9b8dea4...eff55bf
Details
codecov/project 100% remains the same compared to 9b8dea4
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - pom.xml (checkstyle) No new issues
Details
wercker/build Wercker pipeline passed
Details

@rnveach rnveach deleted the rnveach:it_configs branch Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.