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

[bobtiernay-okta]

When developing large suite of ArchUnit rules, one common pattern is using multiple ArchRules / ArchTests in a single test class file:

 @AnalyzeClasses(..)
   class MyArchitectureTest {
       // includes all @ArchTest members from MyArchRuleSuite1
       @ArchTest
       static final ArchTests includedRules1 = ArchTests.in(MyArchRuleSuite1.class);

       // includes all @ArchTest members from MyArchRuleSuite2
       @ArchTest
       static final ArchTests includedRules2 = ArchTests.in(MyArchRuleSuite2.class);
}

However, each included ArchTests may run very slowly and it appears as though it is not possible to only run a subset of these tests from the command line. It would be great if new system properties were exposed that allowed JUnit 4 / 5 runners to only execute rules by pattern, in similar fashion to how JUnit does for test classes. Ideally this would work well with existing tooling for Maven (e.g. Surefire) and Gradle (e.g. test filtering).

One idea is exposing 2 new system properties:

• archunit.tests.includePattern
• archunit.tests.excludePattern

Which support wildcards. e.g.

• my.org.MyArchitectureTest.includedRules1.rule2
• *MyArchitectureTest.includedRules*.rule*

[codecholeric]

Thanks for bringing this idea! :smiley: I agree that this is one annoying part at the moment, personally I also comment the @ArchTest I don't want to execute :zany_face: In theory there would be an API of archunit-junit5-engine that would allow to run single fields just like single methods. But it would likely need a custom IDE / build tool plugin. I would guess from Gradle it would even be possible to patch this in somehow, but it's certainly not easy to use at the moment :disappointed:

[crizzis]

I'm not sure I follow. What is wrong with the existing tooling support? Is it not possible to exclude test classes by category / file name?

Also, what's the point of grouping ArchUnit suites under one uber-suite if you're going to then exclude some of them anyway? How was the grouping helpful in the first place, then?

[codecholeric]

One common way I have used several times is to group rules together and include them hierarchically, like

@AnalyzeClasses(..)
class ProjectOneArchRules {
    @ArchTest
    static final ArchTests serviceRules = ArchTests.in(ServiceRules.class);

    @ArchTest
    static final ArchTests controllerRules = ArchTests.in(ControllerRules.class);
}

This works completely fine in CI where I want to continuously execute that whole suite. But now let's say I'm confused why controller rule 5 is failing suddenly (let's assume the message is not self-explanatory this time :wink:). So now I want to execute only controller rule 5 locally to test it real quick. At the moment this is really tedious to achieve. That's what this issue is about.

[crizzis]

@codecholeric Please have a look at my draft PR when you have a moment.

The PR:

• introduces support for JUnit 4's Assume API
• introduces support for Jupiter's @EnabledIfXxx annotations (for method-flavored tests) and a set of bridging @Conditions.EnabledIfXxx annotations for field-based tests
• introduces support for Jupiter's @Tag annotation for method-flavored tests
• introduces support for Jupiter's @Disabled for method-flavored tests
• enables Surefire test filtering for field-flavored tests (for JUnit Jupiter and JUnit Vintage, but not vanilla JUnit 4, which I believe is a fair tradeoff); note that modifications to Surefire are also needed - see related Jira issue
• enables Gradle test filtering for field-flavored tests, which now seems to work for both JUnit 4 and JUnit 5; note that modifications to Gradle are also needed - see related GitHub issue, which I doubt is the final version of the solution

The reception of the related PR at Surefire was rather frosty, and someone from Gradle commented that they have other priorities at the moment. After closer inspection of the code, though, it seems it would be enough to make FieldSource extend MethodSource, returning the field name from getMethodName, so if things go south with either Surefire or Gradle, this seems like a viable hac*... ekhrm workaround.

One last thing, the PR is a draft mostly because it relies on custom versions of Surefire and Gradle. Once both PRs are merged, the dependency on the custom Surefire version can be lifted. Not entirely sure what to do about the dependency on Gradle, because archunit-junit5-engine now depends on gradle-testing-junit-platform, and Gradle seems to have stopped publishing individual Jars to public repos recently.

[hjohn]

This seems to support JUnit 5 much better than the current version.

I'm having major trouble providing a set of common rules for our projects, but excluding some if a certain project doesn't like one of the standard rules. ArchTests.in doesn't cut it because it does not allow excluding rules, nor does it facilitate having a set of common rules that can be extended without explicitly having to include them in a project.

At a minimum it would be good if ArchUnit would work properly with non-static methods, because then the (abstract) base class could do:

boolean packagesShouldBeFreeOfCyclesRuleActivated = true;

@ArchTest
public void packagesShouldBeFreeOfCycles(JavaClasses classes) {
    assume(packagesShouldBeFreeOfCyclesRuleActivated);

    slices().matching(BASE_PACKAGE_NAME + ".(**)").should().beFreeOfCycles().check(classes);
}

And subclasses can then turn this off if they don't like this standard rule.

Unfortunately, instantiation fails.

JUnit 5 offers tons of options to control which tests to run (Tags, EnabledIf/DisabledIf annotations, Assumptions, TestFactory) but basically in ArchUnit's current form none of them can be used for this use case.

[codecholeric]

The problem is that ArchUnit's TestEngine should not have any dependency on the Jupiter API (which is actually a big problem with #834). The Jupiter API doesn't fully match the model (e.g. fields can't be tests there, so some annotations can't be assigned to fields).

But we can of course implement a reasonable subset in a compatible way. One thing that already is possible though is tagging, which you can do via @ArchTag.

This issue originally was just about a simple test filter, so I feel it has been hijacked a little :thinking: I'm happy to collect possible features with some reasoning for which case they are needed, but we should discuss them first. In particular we should not reuse Jupiter APIs or add a dependency to it. And I would likely create separate issues then with concrete action items.

Maybe you could start by describing how exactly you would want to run your tests and where the current limitations prevent you from doing it in a good way (so we can ponder about the cleanest solution to solve it, I would e.g. avoid inheritance if possible).

[crizzis]

If I may drop in my two cents, the PR can be conceptually divided into two parts: (1) making sure test filtering works with popular test tools, and (2) better support for JUnit 5 and JUnit 4 test exclusion-related API.

TBH even while implementing both parts, I felt that their relative usefulness (given what facilities are already provided by ArchUnit API itself) is strongly in favour of (1).

@codecholeric Since you've expressed concerns over adding a dependency on JUnit 5 API, then perhaps we could cherry-pick the part dealing with (1), and move the discussion about the extent to which we want/need (2) to another issue?

I believe that would provide a way for users to solve 90% of possible use cases, and (2) could be moved to an optional pluggable module, if need be.

WDYT?

(Sorry for making a tangled mess of a PR which I now very clearly see should have been divided into at least two distinct parts, BTW)

[hjohn]

I can understand you don't want JUnit 5 specific stuff in ArchUnit itself, however, if we can just call into ArchUnit from a normal JUnit test (I've looked, but couldn't find a way) then you can use what JUnit already offers to control what tests are run. I feel that annotations like ArchTest and ArchTag are just reinventing what is already available in JUnit.

As for a description of what I think would good to have: I would like to offer a full set of ArchUnit rules in a common project. Projects interested in using those rules would simply create one JUnit test case that uses these common rules (either by subclassing a base class, or some other mechanism like ArchTests.in). They may need to provide some parameters (like base package name).

The problem however starts when a certain project isn't quite ready for the full set of rules, or when a certain rule is controversial. Such rules should be easily disabled. ArchUnit offers inclusion mechanisms only (as far as I could find), but no exclusion mechanism. This is problematic because if rules are added to the commons project they will not be automatically applied to projects importing the common rules (a build may fail, but then those rules can consciously be turned off or fixed). So I think what's needed is:

• A way to offer a set of common rules (imported from another artifact or project)
• A way to run all those rules without having to specify all of them so an upgrade of the common rules would run newly added rules
• A way to exclude some of those rules that projects are not ready for, are controversial or are too hard to fix

This could be solved in several ways, several which I've attempted but failed to get to work:

• Make non-static @ArchTest annotations work in a super class; when applied to a method it would allow to stop the test from running using assume on some condition dictated by a sub class
• ArchTag works similar to JUnit Tag which does not seem to offer a way to include or exclude tests programmatically
• Try calling ArchUnit from a regular @Test; I couldn't find a way to get the required JavaClasses object

Tags could definitely work, but what I'm missing is a way to do something like:

@AnalyzeClasses(packages = ArchitectureTest.BASE_PACKAGE_NAME)
@ArchExcludeTag({"namingConventions", "controversialPackageRule"})  // <--
public class ArchitectureTest {
    static final String BASE_PACKAGE_NAME = "com.acme.commons.core";

    @ArchTest
    private final ArchTests standardRules = ArchTests.in(StandardRules.class);
}

[hjohn]

If I may drop in my two cents, the PR can be conceptually divided into two parts: (1) making sure test filtering works with popular test tools, and (2) better support for JUnit 5 and JUnit 4 test exclusion-related API.

TBH even while implementing both parts, I felt that their relative usefulness (given what facilities are already provided by ArchUnit API itself) is strongly in favour of (1).

@codecholeric Since you've expressed concerns over adding a dependency on JUnit 5 API, then perhaps we could cherry-pick the part dealing with (1), and move the discussion about the extent to which we want/need (2) to another issue?

I believe that would provide a way for users to solve 90% of possible use cases, and (2) could be moved to an optional pluggable module, if need be.

Splitting off the first part is probably wise, smaller PR's are easier to review and get included. The engine shouldn't need to depend on JUnit 5, although some changes could still be needed there to allow the more specific JUnit 5 artifacts to offer more features.

[hankem]

Try calling ArchUnit from a regular @Test; I couldn't find a way to get the required JavaClasses object

You can use the ClassFileImporter, see also ArchUnit-Examples → example-plain.

[codecholeric]

@crizzis Yes, I think that's a good idea, I'll try to do that :+1: @hjohn As @hankem pointed out you can always use ArchUnit directly without any ties to a specific framework, thus using every possibility some framework offers. The tests then look like here. The only thing to keep in mind is that you need to take care of caching the JavaClasses yourself then and you have to watch out not to forget calling ArchRule.check(classes) on your imported classes :wink: Other than that, thanks a lot for your use case, I think I understand the main pain and we'll see how to best solve it!

[crizzis]

@crizzis Yes, I think that's a good idea, I'll try to do that 👍

@codecholeric Any progress on this one? I have a bit of time I could dedicate to this issue right now, so I can try and lend a hand

[codecholeric]

@crizzis Unfortunately not :disappointed: Problem is that I have a ton of other things in the pipeline still...

If you wanna give it a shot, I'd be happy :slightly_smiling_face: But I would suggest we try smaller PRs to stay in sync, e.g. start with something that is really close to the issue. The original issue just wanted the ability to filter @ArchTests by name, e.g. by a simple system property. Which would be flexible and build tool / IDE independent even though not the most convenient. But we need to keep in mind that you might also want to have this filtering in the IDE for example. If you can get Maven / Gradle to reasonable support filtering from the command line via some SPI, that would of course also be great, even though it wouldn't solve the problem when running from the IDE (without going through the build tool). But as mentioned, dependencies on JUnit Jupiter I would want to avoid. So we should not use Jupiter specific annotations. I think it makes sense to chunk it small, then I can also throw in reviews quicker (since they are not so extensive) and we can merge stuff quicker :+1: For example the existing PR adds 3.5K lines of code, which is nothing I can just review in between. Maybe pick one particular option to filter first (optimally one that would solve the issue as described) and only create a PR for that...

[crizzis]

@codecholeric Okay, I will try my best to do this in incremental steps.

First, this PR, which introduces tests for the testing tools (tests for the new functionalities are still disabled at this stage)

(I don't see a way of making this one smaller, but it just adds a new module and does not yet modify production code in a major way, so hopefully it will be easier to review).

(Also, please note the comment inside MavenSurefireEngine#resolveJUnitEngines, I might have identified a possible configuration bug / documentation issue related to using JUnit Vintage alongside ArchUnit, but I'm not sure)

[crizzis]

@codecholeric Also, if you get the time, please feel free to comment on the next PR built on top of the previous one, which introduces support for Surefire. It will require some input from you before it is ready.

Note that modification to Surefire itself is no longer needed.

[codecholeric]

@crizzis thanks for your effort! As I wrote on the PR, what would be important for me is a little bit more reasoning and comparing. Because I don't know the vision in your head where you want to go to. In the end there are different ways to solve the problems stated in the issue, and if we want to go one way, then we need to reason why. Because every solution has pro's and con's, if we go with one we should make it clear why and why we disregarded the other one. For example, why not use the simple idea of a pattern supplied via system properties (that could be easily tested without the need for any tooling test).

[crizzis]

@codecholeric Absolutely.

When starting work on this task I focused on this requirement:

Ideally this would work well with existing tooling for Maven (e.g. Surefire) and Gradle (e.g. test filtering).

I believe this is an important goal from the POV of the principle of least astonishment: the end users are (presumably) already familiar with the facilities their testing tool provides for filtering. In other words: such a system property as the idea in the ticket suggests already exists for Surefire users, for instance: it's called surefire.includes / surefire.excludes. It already follows a certain syntax, has some priority rules in place, etc. No reason to burden the user with yet another filtering mechanism with its own 'mechanics'.

why not use the simple idea of a pattern supplied via system properties

That's also a great idea! It would cater to those end users who use more exotic setup (e.g. call ArchUnit tests programmatically). I would propose to implement it as yet another filter returned from TestSourceFilter.forRequest (see comment).

In fact, I was planning to suggest this as the next PR, but before I jump into it, two decisions would need to be made:

• Priority: if both the tooling property (e.g. surefire.includes) and ArchUnit property (archunit.tests.includePattern) is used, which one takes precedence, do they combine or does one override the other etc.
• Syntax: again, calling on the principle of least astonishment, what sort of syntax should be supported? Glob, regex, the syntax Surefire uses, the syntax Gradle uses, something else entirely?

that could be easily tested without the need for any tooling test

I thought the tooling tests would in and of themselves represent added value but, putting it bluntly, the functionality itself does not depend on them in any way, and I'm not emotionally attached to them ;)

I just assumed it would be nice to verify whether the testing tools play nice with ArchUnit, test filtering or not. Perhaps those tests do not need to be a part of every single build, for example?

Also, as far as Maven goes, I wasn't sure whether or not the new tests overlap in scope with what archunit-maven-test already tests for, perhaps the two should be merged somehow?

Ultimately, it's up to you.

[codecholeric]

I agree that it is nice if it just seamlessly fits into the tooling API, i.e. if we are able to reuse surefire.includes, etc., that would be great! Can you maybe outline your ideas for those two platforms real quick? Like which extension point to use, etc.

From an architecture point of view there are two principles I think we should follow: a) no dependencies from existing modules archunit-junitxxx to any tooling. E.g. no dependency from junit5-engine to Surefire-API. Because JUnit5 Engine should really only depend on the JUnit Platform and be completely comprehensible without any knowledge about any external tool, i.e. users that use Gradle and ArchUnit JUnit 5 should not be forced to pull some dependency on Maven tooling and vice versa. And b) minimal changes to existing JUnit modules for the sake of any tool, i.e. no hacks, etc., that make it harder to understand the JUnit test infrastructure code of ArchUnit. Dependencies should always go from tooling infrastructure into ArchUnit JUnit support and not vice versa. If we would need to make some compromise between usability and dependencies, then I would suggest to make any tooling support completely optional, fully encapsulated and only active in some reflective way (e.g. if class x is on the classpath, then plug in an additional adapter, something like that, but nothing that would hard pull in any additional dependency when a user adds junit 5 to the build dependencies. In other words some completely separated tooling dependent classes and the core as it is now, not runtime-dependent on any tooling in any way)

Regarding tooling test, I don't feel I can decide it yet, because I have no complete vision how the implementation will look like. Normally I would say our interface should be the JUnit 4 / JUnit Platform interface and we expect that tools behave consistently and otherwise file bugs with the tools whenever something pops up (as I have e.g. done with Gradle in the past). If we actually decide to implement tool dependent parts, then it likely makes sense to test these somehow, so in this case it would IMHO make sense.

As for the syntax, I would start with the simplest wildcard syntax for the system property: Exact match for all characters except the asterisk * which matches arbitrary many characters :man_shrugging: That would be consistent with Gradle test filtering AFAIS. We could also name the system property archunit.junit.includeTestsMatching to be even closer to the Gradle naming. I realize that Surefire goes down a different path with a more complex matching syntax, but it also says that one is only supported for JUnit 4 and TestNG (at least that's how I read the docs, haven't really used Maven with JUnit 5 much). IMHO the simple strategy matches the more generic descriptor of JUnit 5 nicer :man_shrugging: But if we ever want something more complex, we can just add a second system property, like archunit.junit.includeTestsMatchingRegex or something like that...

Considering the implementation strategy I'm wondering if it wouldn't make sense to implement the system property version first. If we decide we'll do that anyway. Because I think it's simpler and it doesn't require any extra tooling. Tests can just be added to the existing tests :thinking: And it would already provide a much desired fix for the current problem. Hooking into Surefire and Gradle test filtering would be super nice, but for me it's icing on the cake :wink:

[codecholeric]

Ah, I forgot regarding priority if we should at some point support system property + tooling filters: I would combine these filters the same way as if multiple filters would have been specified in tooling. E.g. two includes I would AND together...

[crizzis]

Ok, you've raised quite a lot of points, so let me try and unpack them step by step:

Can you maybe outline your ideas for those two platforms real quick?

The idea behind the platform support is best understood by looking at AbstractTestNameFilter and its concrete implementation, SurefireTestNameFilter

The implementation will be strikingly similar for both platforms:

• first, TestSourceFilter.forRequest will invoke each AbstractTestNameFilter implementation's appliesTo method, which reflectively checks whether the test was launched with the given tool (by inspecting the EngineDiscoveryRequest that originated the test invocation for tool-specific post-discovery filter). As an example, Surefire's post-discovery filter implementation is called org.apache.maven.surefire.junitplatform.TestMethodFilter, and so SurefireTestNameFilter.appliesTo will test whether the request contains a filter with this class name (note no direct dependency on Surefire yet)
• if the post-discovery filter is found, then the object returned from TestSourceFilter.forRequest will simply delegate to the underlying filtering mechanism of Surefire's / Gradle's post-discovery filter to check which tests should run. In the above PR, this is the resolver.shouldRun call in line 55. Note that here we are depending on Surefire API, extracting TestListResolver from TestMethodFilter using reflection and calling it directly. This call can, in principle, be done reflectively, at the expense of compile-time safety
• if no filter is found (i.e. the user is not using any platform-specific filtering), what is returned from TestSourceFiler.forRequest is basically a no-op (accept-all) filter
• finally, the returned filter is applied whenever an ArchUnit test descriptor creates child descriptors (only creating those child descriptors that would match the testing platform's filter - see changes in ArchUnitTestDescriptor)

Of course, when handling @ArchTest, only the name of the @ArchTest- annotated field is matched against the filter, and if the match is positive, the filter is not applied to the child descriptors. This way, filtering can be easily applied to both test cases and test suites.

Like which extension point to use, etc.

If you mean which Gradle / Surefire extension point to use, then the answer is none. Neither platform exposes extension points that would let us provide any support whatsoever for filtering any test source other than MethodSource. This is precisely why the first approach at a solution to this ticket was to add such an extension point to both platforms. It was only after finding it problematic to convince Surefire folks to add such an extension point, and then realizing we could use reflection to inspect the original launcher request instead, that I figured out the above solution.

I would suggest to make any tooling support completely optional, fully encapsulated and only active in some reflective way

This would certainly be possible, but please have a look at the PR and identify the parts of it that you find problematic, and we can continue this discussion in the PR without digging into too much technical details here.

(for instance, I assumed a dependence on Surefire API exclusively, as opposed to the entire Surefire implementation, would be an acceptable compromise, but if you decide otherwise, that dependency can be eliminated still. I just thought applying reflection all the way through would obscure the picture a bit too much)

Regarding tooling test, I don't feel I can decide it yet

Should I rebase the few changes I've done to production code in this PR onto this one, then?

we expect that tools behave consistently and otherwise file bugs with the tools whenever something pops up

I would wager to say that this chimes pretty well with my suggestion of the tests not being part of every single build. Perhaps they could be run on demand, whenever a new version of the tooling platform is released, precisely to see if something 'pops up'?

As for the syntax, I would start with the simplest wildcard syntax for the system property: Exact match for all characters except the asterisk * which matches arbitrary many characters

Should it match both qualified and simple class names? I believe the Gradle syntax supports that

Considering the implementation strategy I'm wondering if it wouldn't make sense to implement the system property version first.

I can roll out another PR with this implementation, but I'm still going to need the base functionality of TestSourceFilter from the Surefire PR. I can try to extract that when I have some time ;)

[crizzis]

@codecholeric As promised, I implemented the system property version. Here's a draft pull request

(Also, please ignore this one, I created it by mistake and can't seem to delete it - probably a permissions issue)