[FLINK-27476][test] build import option only focuses on maven main class files

[JingGe]

What is the purpose of the change

ImportOption.DoNotIncludeTests.class used currently has some issue when running test with testContainer. It would be good to define the target class path precisely.

The excluding style import option just excludes some known paths which is not a stable solution. It will break if, in this case, any test classes are found in other path. While running the same archunit test 1. alone via maven, 2. in Idea, 3. with ITCase which needs testContainer together via maven for the external connector repo FLINK-27061, only the third one failed.

Brief change log

• build new import option MavenMainClassesOnly
• remove production code filter used in rule definition.

Verifying this change

ImportOptions.MavenMainClassesOnly.class could be used within @AnalyzeClasses(...)

Does this pull request potentially affect one of the following parts:

• Dependencies (does it add or upgrade a dependency): (yes / no)
• The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
• The serializers: (yes / no / don't know)
• The runtime per-record code paths (performance sensitive): (yes / no / don't know)
• Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
• The S3 file system connector: (yes / no / don't know)

Documentation

• Does this pull request introduce a new feature? (yes / no)

[flinkbot]

CI report:

• 9827d95 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[zentol]

Could you expand on what concrete issue you're trying to solve?

There is supposed to be some issue with testcontainers; what exactly is it?

[JingGe]

Could you expand on what concrete issue you're trying to solve?

There is supposed to be some issue with testcontainers; what exactly is it?

I got issue while testing the external ES connector. The excluding style import option just excludes some known paths which is not a stable solution. It will break if, in this case, any test classes are found in other path. I have run the same archunit test 1. alone, 2. in Idea, 3. with ITCase which needs testContainer together. Only the third one failed.

[JingGe]

@flinkbot run azure

[zentol]

Can you give me an example for one such "other path" (and ideally a violation as well)? I'm curious why we'd have issues now with the external repo when the same tests seem to work just fine in the flink repo.

[JingGe]

In the external connector repo, the ProductionCodeArchitectureTest has been moved to sub modules[1]. That's why we don't have the same issue in the flink repo. Violations could be found at [2].

[1] https://github.com/apache/flink-connector-elasticsearch/pull/10/files#diff-2f93acbdbdf55f0be081d4c7760ed5bd67706ab609b0657b63a8c031dd4dd55a
[2] https://github.com/apache/flink-connector-elasticsearch/runs/6206623054?check_suite_focus=true#step:6:695

[zentol]

So where does archunit pick up the es-base classes from? Does it just read the classpath?

I see that SourcePredicates#areProductionCode selects everything that doesn't match certain (test) paths; so if it scans a jar this naturally fails.
Should we maybe adjust SourcePredicates#targetCode to additionally filter everything coming from a jar?

[zentol]

Like this:
.map(location -> !location.isJar() && predicates.includes(location))

[JingGe]

there are other ImportOptions that are working on jar and archive: ImportOption.DoNotIncludeJars and ImportOption.DoNotIncludeArchives. I think the current SourcePredicates#targetCode is fine and could be used with them together, if we want to exclude jars.

[zentol]

Have you tried whether ImportOption.DoNotIncludeJars would fix the issue?

[JingGe]

Have you tried whether ImportOption.DoNotIncludeJars would fix the issue?

Yes, that could fix it, thanks! And I think the import option I built in this PR is still very helpful for the future development, because it is more precise. WDYT?

[zentol]

I don't necessarily mind it, but do think that we should figure out the responsibilities a bit.

Currently, a test can select certain classes to test via the ImportOptions. This newly added import option would push the responsibility of selecting production code to the test.
But then we have rules (like the Visibility thing) that select production code on their own.

So who's responsible for selecting production source code?

And if we're going with both the user and rule having the capability to do so, then they should rely on the same logic.

[JingGe]

Conceptually, you are right. As far as I understood, they have two different responsibilities. SourcePredicates are used to build the rules with their default classes scope. Same Rule will be used in different tests and each test will have its individual classes scope via @AnalyzeClasses(...ImportOptions...)and might be different from case to case even using the same rule. In the end, both of them together are responsible for defining the classes scope when the test is running.

[JingGe]

@Airblader After talking with @zentol offline and checking the archunit doc again. I think, theoretically, class path should only be managed by the test via @AnalyzeClasses(...ImportOptions...). It should work without adding the SourcePredicates#areProductionCode logic into the rule definition. There might be some corner cases I am not aware of. May I have your thoughts of why developing the SourcePredicates? Is there anything that SourcePredicates could do but customized ImportOptions could not do?

[Airblader]

Originally this was done through the predicate because the idea was to build the class graph only once. Since test code is now tested per-module anyway, I don't see a reason why this couldn't instead be done on the classpath scanning level already.

[JingGe]

Thanks for the feedback. If I understood you correctly, we should keep the SourcePredicates#areProductionCode in the production code test rule definition to provide better performance, shouldn't we?

[zentol]

If you use the new import option the source predicate filters nothing; if anything it should cost performance. For the Flink production tests it also filters nothing, so it's again costing us.

[JingGe]

Thanks for your feedback @zentol, I addressed your comments, please take a look.