Rework IT's Test Structure

[rnveach]

Continuing on #11603 and #11585 ,

Google and Sun IT's tests follow a different pattern than XPath and Test. Because of this, both test areas should change their test make up based on their respective testing pattern.

This is planned to be test/it only changes, nothing in main will change. It also will be a multi-PR work (4 PRs are planned).

=============

Examples:

Google and Sun does regression on an existing configuration. They do not create modules from memory or scan existing modules. Their methods should revolve around this.

checkstyle/src/it/java/com/google/checkstyle/test/base/AbstractGoogleModuleTestSupport.java

Line 41 in 74747a2

private static final Set<Class<?>> CHECKSTYLE_MODULES;

checkstyle/src/it/java/com/google/checkstyle/test/base/AbstractGoogleModuleTestSupport.java

Line 52 in 74747a2

CHECKSTYLE_MODULES = CheckUtil.getCheckstyleModules();

We should remove reliance on ModuleCreationOption and rework the code to know the hierarchy similar to test as we follow the configuration file.

checkstyle/src/it/java/org/checkstyle/base/AbstractItModuleTestSupport.java

Line 58 in 74747a2

public enum ModuleCreationOption {

In addition, their tests will need changes related to the changing result of the getModule methods which will return multiple, nested configs.

checkstyle/src/it/java/com/google/checkstyle/test/chapter4formatting/rule462horizontalwhitespace/GenericWhitespaceTest.java

Line 42 in 74747a2

final Map<String, String> messages = checkConfig.getMessages();

checkstyle/src/it/java/com/google/checkstyle/test/chapter5naming/rule528typevariablenames/RecordTypeParameterNameTest.java

Line 41 in 74747a2

final String format = configuration.getProperty("format");

XPath's regression very closely follow's test, so it should extend test's structure.

Similar changes to IT and XPath should be copied over to test like usual.

[romani]

I am missing a point to depend less on config, whole idea of such tests is to test config as whole. Users do not care about how much modules we have in config.

[rnveach]

@romani See PR #11652 to understand this point. Let me know if you could point to where the confusion may be.

Tests should not pull information in from the config to build it's results. They should be more independent to actually test the config and the reason is in a future PR I will change the result of the current getModuleConfig. This kind of change will thus cause a simple checkConfig.getMessages() to have to be changed to something like checkConfig.getChildren()[0].getMessages().

The current PR is just building the ground work for it and to keep the PRs more easily reviewable. I could push all 4 PRs now, if you want to see the completed rework of IT but they each have their dependencies on others.

Users do not care about how much modules we have in config.

This whole issue is connected to reworking our tests. No code in main is planned to be changed. So our users who use our release won't see any difference.

The main focus will be IT, but I do have some minor connected changes for test.

[nrmancuso]

@rnveach after thinking on this a bit and reviewing your PR at #11652, I understand the motivation behind this issue. It does seem like it could be problematic to pull values from the config we are testing. But, perhaps I do not understand the point of the Google style tests. Are we testing the content AND behavior of the config? If so, then it makes sense not to pull values from the config itself. But, if we are only verifying the behavior of Checkstyle with the google style config and do not care about the actual content, then it makes sense to just pull the values from the config so that we only update in one place.

@romani

whole idea of such tests is to test config as whole

I agree with the above. Are we testing the behavior only, or also the content of the config? If we are also testing the content of the config, it might make sense to separate out certain elements of the config so that we have "actual" (content in config), and "expected" (values in tests).

[romani]

Users do not care about content of config. They want it to work and cover guideline. It doesn't matter how much modules (even inline filters for suppression) are used to cover a specific chapter from guideline.

Test of modules are done in regular test classes. We need IT tests of guideline chapters. But current IT were our first step in bdd mentality, so them still have unit way of thinking.

[rnveach]

It does seem like it could be problematic to pull values from the config we are testing.

PR #11652 is blocked because it will be reworked to what I talked with from romani to provide a method to easily obtain the values from the config.

the point of the Google style tests

Basically regression to show our config follows the standards of Google. One issue I have though is the test only runs part of the config and not the full config. I have no plans to change that right now.

Are we testing the content AND behavior of the config?

You can't really test behavior without also touching on the content, so I am confused by the question.

[romani]

if we ever endup in refactoring of this, we need to test by chapter from guideline.
if we map chapter requirements to set of modules, we need to load them from google_checks.xml and run then together over Input file.
I added it to https://github.com/checkstyle/checkstyle/wiki/Checkstyle-GSoC-2024-Project-Ideas#project-name-update-google-style-config-to-most-recent-content-of-style-guide-and-resolve-known-issues-with-modules-that-are-in-such-config

There should be no dependency to Xpath tests where we do single module execution.

[romani]

I approved issue, but there might be nuances as some wording is not clear for me.

But yes, Xpath is regular single module test, old style.
ITs are multi-module tests that serve some business goal defined in some human written guideline and cover some part of it.