Feature/issue 641 filtering env props #899

crizzis · 2022-06-23T17:02:04Z

No description provided.

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

codecholeric · 2022-06-26T07:43:50Z

archunit-junit/junit5/engine/build.gradle

 }

 ext.moduleName = 'com.tngtech.archunit.junit5.engine'

-ext.minimumJavaVersion = JavaVersion.VERSION_1_8
+ext.minimumJavaVersion = JavaVersion.VERSION_1_9


We can't do this or we will break archunit-junit5 support for all users out there that use Java 8!

Ummm...

The getting started doc for ArchUnit says:

ArchUnit can be obtained from Maven Central.

Gradle
dependencies {
testImplementation 'com.tngtech.archunit:archunit:0.23.1'
}

and (unless I misread something) the archunit module already declares minimumJavaVersion to be 9, so I was under the impression ArchUnit already dropped support for Java 8

Anyway, I can adapt the code to use Java 8, it's just unclear to me what the official supported minimum Java version is, could you please comment on that? It would have been nice to have it mentioned in the contributing guide...

All previous releases (i.e. up to 0.23.1) actually support Java 7; its support has only recently been dropped with #833. 😉

minimumJavaVersion is only set to JavaVersion.VERSION_1_9 (or JavaVersion.VERSION_16) for individual Gradle tasks compileJdk9mainJava, compileJdk9testJava, jdk9Test (or compileJdk16testJava, jdk16Test, respectively).

Duly noted.

I've downgraded the version back to Java 8 (turns out Java 9 features are not yet used in this PR), but please consider adding this to the contributing guide as it's pretty confusing, especially given that some modules use ext.minimumJavaVersion = JavaVersion.VERSION_1_9

codecholeric · 2022-06-26T09:08:29Z

archunit-junit/junit5/engine/build.gradle

    implementation project(path: ':archunit-junit')
    dependency.addGuava { dependencyNotation, config -> implementation(dependencyNotation, config) }
    implementation dependency.slf4j
+    implementation dependency.junitPlatformLauncher


We should not add any dependencies on unrelated JUnit projects. Only depend on the platform. I think this would also be a strange dependency, since the Platform Launcher launches TestEngines, so a dependency back from TestEngines to the launcher feels wrong to me 🤷‍♂️

In that case, please have a look at AbstractTestNameFilter.findPostDiscoveryFilter. We need a dependency on Launcher to be able to cast the discovery request to LauncherDiscoveryRequest, and then to use the result non-reflectively.

(you can see how the result is being used in the Surefire PR. note that in short, this is the price that needs to be paid for not introducing changes to Surefire itself)

If you insist on removing this dependency, please suggest a way to obtain the same result. We can, for instance, use reflection all the way through, I was just trying to maintain some level of compile-time safety.

(Also, if polluting the end-user's classpath is your concern, please keep in mind that the Platform Launcher is going to be the first dependency each and every testing platform brings in. I would have made it a compile-only dependency, if not for the fact Gradle is very strict about separate classloaders for user and framework code)

What I don't understand is why we would need any dependencies on other libraries or tooling at all to solve this problem. The first issue is simply: "Given a system property archunit.junit.includePattern, only execute tests that match the pattern", right?
Say I

Convert the pattern to a filter for (ArchUnit)TestDescriptor (we could e.g. analyse the UniqueId, which in the end is also just a concatenation like .../[class:some.SomeClass]/[field:someField] that matches the info we have in the pattern very closely; just not sure how stable this is; otherwise we could add a custom property to (Abstract)ArchUnitTestDescriptor or sth. like that)

Within com.tngtech.archunit.junit.internal.ArchUnitTestEngine.discover(..) once all the discoveries are through, right before I return the result, I add something like

result.accept(descriptor -> { if (!includeFilterMatches(descriptor)) { descriptor.removeFromHierarchy(); } });

Wouldn't that solve the problem? Tests I could write very easily as part of ArchUnitTestEngineTest.Discovers:

// GIVEN filter is set via system property System.setProperty("archunit.junit.includePattern", "*SomeClass*"); // WHEN classes are discovered testEngine.discover(/* discovery request that would normally include SomeClass */); // THEN classes matching the system property filter are excluded Set<String> displayNames = descriptor.getChildren().stream().map(TestDescriptor::getDisplayName).collect(toSet()); assertThat(displayNames).doesNotContain("SomeClass");

Maybe I'm missing something, but to me it looks like this could solve the issue without any need for additional test infrastructure or dependencies on other 3rd party libraries? 🤔

Convert the pattern to a filter for (ArchUnit)TestDescriptor (we could e.g. analyse the UniqueId, which in the end is also just a concatenation like .../[class:some.SomeClass]/[field:someField] that matches the info we have in the pattern very closely; just not sure how stable this is; otherwise we could add a custom property to (Abstract)ArchUnitTestDescriptor or sth. like that)

Within com.tngtech.archunit.junit.internal.ArchUnitTestEngine.discover(..) once all the discoveries are through, right before I return the result, I add something like result.accept(descriptor -> { if (!includeFilterMatches(descriptor)) { descriptor.removeFromHierarchy(); } });

Well, um, yes, this is essentially what this PR does. The only difference is it filters child descriptors before they are added, and not afterwards.

Tests I could write very easily as part of ArchUnitTestEngineTest.Discovers

Sure, I've already done that. Have a look at the relevant tests.

this could solve the issue without any need for additional test infrastructure or dependencies on other 3rd party libraries

Yes, absolutely. This PR does not depend on 3rd party libraries to be able to perform the filtering. It depends on 3rd party libraries solely because:

I committed some of the code in advance as it's kind of hard to keep 3 PRs in line, especially when one defines an abstract base class for use with the other ones :(

I can try to isolate the changes needed in the other PRs, but this will require splitting some commits. It will also take a while. Are you OK with that?

Sorry for the delay 😬 I was a little occupied to do that 1.0.0-rc1 release. Yes, please, if you don't mind! Make the first PR as small and simple as possible. Also, if we don't need all that test infrastructure code yet, it's also good, because then we can delay the decision further. The smaller and simpler the change, the easier it is to understand later on when we look back at the history. Also a small code change is easy to argue for, a big one needs to be pondered really carefully, if it brings the ROI on the maintenance costs it will cause.

codecholeric · 2022-06-26T09:10:44Z

...unit/junit5/engine/src/main/java/com/tngtech/archunit/junit/internal/ArchUnitTestEngine.java

@@ -67,6 +72,22 @@
 public final class ArchUnitTestEngine extends HierarchicalTestEngine<ArchUnitEngineExecutionContext> {
    static final String UNIQUE_ID = "archunit";

+    private static final Collection<String> ABORTING_THROWABLE_NAMES = Arrays.asList(
+            "org.junit.internal.AssumptionViolatedException",


Why would we support JUnit 4 exceptions within the JUnit 5 support? 🤔 Besides that .internal. anyway sounds in a way that we should not handle it 🤷‍♂️

Those are all the flavors of assumption violated exceptions that can be thrown to indicate assumption violation (i.e. exceptions that should result in skip results, not failures).

As to whether we should be handling the exception or not, please take a look at JUnit's OpenTest4JAndJUnit4AwareThrowableCollector.

Why would we support JUnit 4 exceptions within the JUnit 5 support?

Remember that you can run JUnit 4 ArchUnit tests using the ArchUnitEngine (NOT Vintage!)

Hmm, but in the link the comment has a question mark 🤔 I don't think we really need to support this for ArchUnit, because I don't think users should use JUnit 4 things within the scope of the ArchUnitTestEngine.

I also don't understand completely what you mean by "Remember that you can run JUnit 4 ArchUnit tests using the ArchUnitEngine", I don't think that is the case? 🤔 JUnit 4 @ArchTests run with a custom JUnit 4 runner, also the relevant annotations are separate, e.g. JUnit 4 support has a different annotation @AnalyzeClasses than JUnit 5 support has.

JUnit 4 support has a different annotation @AnalyzeClasses than JUnit 5 support has.

Different how? They have the same name and package.

This is exactly the configuration issue I was trying to tell you about. If you run your tests with both archunit and vintage engines, the tests are being run twice. They are eligible for discovery by both engines. Try debugging the tooling tests with both engines on, you will see both engines executing the test cases.

I don't think users should use JUnit 4 things within the scope of the ArchUnitTestEngine.

The question is do users know that? If they use the method flavor of archunit tests, they are allowed to use assertThat. Why wouldn't they be allowed to use assumeThat?

(Also the users don't know what ArchUnitTestEngine is, that ArchUnit implements a custom JUnit engine is an implementation detail of archunit)

Different in being separated into different artifacts (it is not supported to use archunit-junit4 and archunit-junit5 together on the classpath) and the @AnalyzeClasses of archunit-junit5 depends on JUnit 5's @Testable while the archunit-junit4 one does not...

I would understand if people use assumeThat(..) within an archunit-junit4 test, but not within an archunit-junit5 test 🤔

I don't know exactly what you mean by "the tests are being run twice". The Vintage Engine will not pick up any @ArchTest tests and the ArchUnitTestEngine or the ArchUnitRunner will not pick up any @Test methods, no? 🤔

it is not supported to use archunit-junit4 and archunit-junit5 together on the classpath

Well, I mean... that changes things. A lot :p

I was talking about the scenario where you have a project with JUnit 4 arch tests, with both archunit and junit-vintage engines enabled. In that case, each test gets picked up by junit-vintage (because of @RunWith(ArchunitRunner.class)) and by archunit (because of @AnalyzeClasses). You can't have both ArchUnitRunner and archunit engine on the classpath, though, while not simultaneously depending on both archunit-junit dependencies. So that makes sense to me now.

Where exactly do the docs say that mixing archunit-junit4 and archunit-junit5 is not supported, btw? I read the user guide on junit support, but I couldn't find it anywhere...

In that case, I now realize I wrongly assumed support for test filtering was working with junit-vintage when actually it just happened to work because the archunit engine was the one that was running the tests :(

...which means there is no point in any support for junit 4 stuff in this or any of the other PRs. I will try to update the PRs accordingly next week. Thanks for your patience:)

No worries 🙂 I didn't add that anywhere in the user guide, because I didn't think it would make sense or be a thing to easily get wrong (since there hasn't been an issue about it in all the years I would hope that also really is the case). If I use the Vintage engine, then I have JUnit 5 in my dependencies. But then why would I include JUnit 4 support if there also is JUnit 5 support 🤷‍♂️ In any case, using Java 9 modules it would also throw an exception, because both the artifacts export the same package. If you use the regular classpath and include both, then I guess there could be some strange behavior instead. But then again, I don't see how anybody would read the user guide, that clearly states if you use JUnit 4 -> include this, if you use JUnit 5 -> include that, then add all dependencies and then go for writing tests using the Vintage engine. It just doesn't make sense. In particular, since a migration from JUnit 4 to 5 is also just dropping the @RunWith line from all tests 🤷‍♂️

codecholeric · 2022-06-26T09:13:46Z

...gine/src/main/java/com/tngtech/archunit/junit/internal/filtering/AbstractTestNameFilter.java

+        List<PostDiscoveryFilter> filters = new ArrayList<>(request.getPostDiscoveryFilters());
+        filters.remove(postDiscoveryFilter);
+        filters.add(replacement);
+        ReflectionUtils.setField(discoveryRequest, "postDiscoveryFilters", filters);


This feels wrong to me 🤔 The JUnit Platform should provide extension points to filter without the need to break encapsulations of the platform in such extreme ways (even having to break the final modifier)

This replaceFilter will only be used for Gradle. I can remove it for now. Sorry, but I commited some of the code in advance as it's kind of hard to keep 3 PRs in line, especially when one defines an abstract base class for use with the other ones :(

I can explain the need for this workaround and what we could do to get rid of it in the Gradle PR, would that be okay with you?

(also, I'm not sure how to respond to 'JUnit Platform should provide extension points' - I'm really working with what I have at my disposal here)

codecholeric · 2022-06-26T09:44:42Z

Thanks for your hard work! I just had a quick look and added some comments for things I noticed at first sight. One important thing to keep in mind is that this is a library, not an application. I.e. we need to be extra conservative about dependencies in production code. And we can't freely adjust the required Java version, etc.

Also, can you explain why we need the tooling tests for this change already? I would imagine that this should be easily testable in a blackbox way by just extending e.g. the ArchUnitTestEngineTest class with tests that basically do

// before
System.setProperty(ARCHUNIT_INCLUDE_FILTER, "some*crazyFilter.*method")

// when -> test that discovery filters out non-included tests

crizzis · 2022-06-26T11:04:24Z

Thanks for the feedback!

One important thing to keep in mind is that this is a library, not an application. I.e. we need to be extra conservative about dependencies in production code. And we can't freely adjust the required Java version, etc.

I really tried to keep new dependencies to a bare minimum, but please take a look at my responses - I suggested workarounds for some of the points you've raised.

Also, can you explain why we need the tooling tests for this change already?

This is really pending your decision. But, should you decide to go for the new tests, then they must be up to date with the current implementation (e.g. exclude Surefire filtering tests until Surefire is merged etc.)

EDIT I just realized this is the PR I suggested that you ignore because I created it by mistake. If you want to review just the changes to current codebase, without the unrelated files, Here is the one that I initially linked to.

I would imagine that this should be easily testable in a blackbox way by just extending e.g. the ArchUnitTestEngineTest class with tests that basically do
// before
System.setProperty(ARCHUNIT_INCLUDE_FILTER, "some*crazyFilter.*method")

// when -> test that discovery filters out non-included tests

I agree that it would be super easy to just test it inside ArchUnitTestEngineTest. Though, if you decide to go for the new tests, then the same feature will be tested twice. If that's okay with you, and you feel a test inside ArchUnitTestEngineTest is needed anyway, please let me know.

EDIT I also noticed that all my original comments are gone after the force-push-signoff maneuver :| let me try to re-add a couple of explanatory comments

EDIT# no. 2 Added relevant test to ArchUnitTestEngineTest; also, fixed a couple of bugs

crizzis · 2022-06-26T11:28:05Z

...c/main/java/com/tngtech/archunit/junit/internal/filtering/ArchUnitPropsTestSourceFilter.java

+            String escaped = SPECIAL_REGEX_CHARS.matcher(pattern).replaceAll(ESCAPED_INPUT);
+            escaped = MULTIPLE_ESCAPED_ASTERISK.matcher(escaped).replaceAll(SINGLE_ESCAPED_ASTERISK_STRING);
+            escaped = SINGLE_ESCAPED_ASTERISK.matcher(escaped).replaceAll(MATCH_ALL_CHARACTERS);
+            return Pattern.compile("(?:^|\\.)" + escaped + "$", Pattern.CASE_INSENSITIVE | Pattern.UNICODE_CASE);


In case the ^|\\. needs explanation, ^ is supposed to match FQ names, while \\. is supposed to match simple names (i.e. match the pattern starting from a dot, presumably the final dot in a package name)

Also, couldn't decide on the case sensitivity. Please make a decision (arguably, a case-sensitive match would make more sense since Java identifiers are case-sensitive themselves...)

crizzis · 2022-06-26T11:32:51Z

archunit-junit/junit5/engine/src/testFixtures/java/com/example/SomeClass.java

+
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;
+
+public class SomeClass {


Those files need to exist in order for FieldSource.from / FieldSelector.selectField to not throw NoClassDefFoundErrors

crizzis · 2022-06-26T11:41:33Z

archunit-junit/junit5/engine/build.gradle

@@ -62,7 +68,3 @@ def configureDependencies = { deps ->
    }
 }
 this.with project(':archunit-junit').configureJUnitArchive(configureDependencies)
-
-singlePackageExport {
-    exportedPackage = 'com.tngtech.archunit.junit.internal'


@codecholeric Could you please suggest what I should do about this one?

If the other PRs are merged, there's going to be a lot more filtering stuff, so I kind of felt it would be clearer if those resided in a separate subpackage...

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

codecholeric · 2022-07-08T09:44:39Z

| EDIT# no. 2 Added relevant test to ArchUnitTestEngineTest; also, fixed a couple of bugs

If there are bugs in there it would be cool if you could create a PR only fixing that instead of mixing it. Then we could trace it way easier 🙂 Also bugs we should fix ASAP without waiting for a full feature to be done...

crizzis · 2022-07-11T10:47:02Z

| EDIT# no. 2 Added relevant test to ArchUnitTestEngineTest; also, fixed a couple of bugs

If there are bugs in there it would be cool if you could create a PR only fixing that instead of mixing it. Then we could trace it way easier 🙂 Also bugs we should fix ASAP without waiting for a full feature to be done...

Sorry, I meant bugs within the solution, not existing bugs in ArchUnit

codecholeric · 2024-04-10T22:41:02Z

I implemented a simplified version in #1280. Since I have no capacity to work on this further, I'm gonna close this. Feel free to reopen if you want to pick this up or add further improvements.

Added tooling tests

604c899

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

crizzis force-pushed the feature/issue-641-filtering-env-props branch from 98b94a6 to 5547f57 Compare June 23, 2022 17:02

crizzis added 4 commits June 23, 2022 19:06

Base filtering mechanism

2882924

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

Base filtering mechanism (2)

482fac8

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

Support for filtering via system properties

5849332

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

Fixed matching by simple class name

2252dfb

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

crizzis force-pushed the feature/issue-641-filtering-env-props branch from 5547f57 to 7429227 Compare June 23, 2022 17:06

crizzis mentioned this pull request Jun 23, 2022

Add ability to execute a subset of rules from the via system properties #641

Open

codecholeric reviewed Jun 26, 2022

View reviewed changes

crizzis commented Jun 26, 2022

View reviewed changes

crizzis added 2 commits June 27, 2022 10:15

Fixed test config for JUnit 4 (only execute via Vintage)

f08053f

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

Fixed baseline tests, downgraded Java version

d064bc2

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

crizzis force-pushed the feature/issue-641-filtering-env-props branch from c3c4f39 to d064bc2 Compare June 27, 2022 08:16

crizzis added 4 commits June 27, 2022 10:54

Fixed test config

bec9d29

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

Fixed test config (2)

fd50877

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

Bugfixes

ac37117

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

Added test engine test for filtgering via config properties

b26665b

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>

codecholeric closed this Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue 641 filtering env props #899

Feature/issue 641 filtering env props #899

crizzis commented Jun 23, 2022

codecholeric Jun 26, 2022

crizzis Jun 26, 2022 •

edited

Loading

hankem Jun 26, 2022

crizzis Jun 27, 2022

codecholeric Jun 26, 2022

crizzis Jun 26, 2022 •

edited

Loading

codecholeric Jul 8, 2022

crizzis Jul 11, 2022 •

edited

Loading

codecholeric Jul 24, 2022

codecholeric Jun 26, 2022

crizzis Jun 26, 2022 •

edited

Loading

codecholeric Jul 7, 2022

crizzis Jul 7, 2022 •

edited

Loading

codecholeric Jul 8, 2022 •

edited

Loading

crizzis Jul 8, 2022

codecholeric Jul 10, 2022

codecholeric Jun 26, 2022

crizzis Jun 26, 2022 •

edited

Loading

codecholeric commented Jun 26, 2022

crizzis commented Jun 26, 2022 •

edited

Loading

crizzis Jun 26, 2022 •

edited

Loading

crizzis Jun 26, 2022

crizzis Jun 26, 2022

codecholeric commented Jul 8, 2022

crizzis commented Jul 11, 2022

codecholeric commented Apr 10, 2024


		import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;

		public class SomeClass {

Feature/issue 641 filtering env props #899

Feature/issue 641 filtering env props #899

Conversation

crizzis commented Jun 23, 2022

Choose a reason for hiding this comment

crizzis Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crizzis Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crizzis Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crizzis Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crizzis Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

codecholeric Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crizzis Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

codecholeric commented Jun 26, 2022

crizzis commented Jun 26, 2022 • edited Loading

crizzis Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecholeric commented Jul 8, 2022

crizzis commented Jul 11, 2022

codecholeric commented Apr 10, 2024

crizzis Jun 26, 2022 •

edited

Loading

crizzis Jun 26, 2022 •

edited

Loading

crizzis Jul 11, 2022 •

edited

Loading

crizzis Jun 26, 2022 •

edited

Loading

crizzis Jul 7, 2022 •

edited

Loading

codecholeric Jul 8, 2022 •

edited

Loading

crizzis Jun 26, 2022 •

edited

Loading

crizzis commented Jun 26, 2022 •

edited

Loading

crizzis Jun 26, 2022 •

edited

Loading