-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add ability to execute a subset of rules from the via system properties #641
Comments
Thanks for bringing this idea! 😃 I agree that this is one annoying part at the moment, personally I also comment the |
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? |
One common way I have used several times is to group rules together and include them hierarchically, like
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 😉). 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. |
Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>
@codecholeric Please have a look at my draft PR when you have a moment. The PR:
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 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 |
Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>
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. At a minimum it would be good if ArchUnit would work properly with non-static methods, because then the (abstract) base class could do:
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. |
The problem is that ArchUnit's 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 This issue originally was just about a simple test filter, so I feel it has been hijacked a little 🤔 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). |
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) |
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 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 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:
This could be solved in several ways, several which I've attempted but failed to get to work:
Tags could definitely work, but what I'm missing is a way to do something like:
|
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. |
You can use the |
@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 |
Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>
@crizzis Unfortunately not 😞 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 🙂 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 |
@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 |
@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. |
@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). |
@codecholeric Absolutely. When starting work on this task I focused on this requirement:
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
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 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:
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 Ultimately, it's up to you. |
I agree that it is nice if it just seamlessly fits into the tooling API, i.e. if we are able to reuse From an architecture point of view there are two principles I think we should follow: a) no dependencies from existing modules 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 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 🤔 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 😉 |
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... |
Ok, you've raised quite a lot of points, so let me try and unpack them step by step:
The idea behind the platform support is best understood by looking at The implementation will be strikingly similar for both platforms:
Of course, when handling
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
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)
Should I rebase the few changes I've done to production code in this PR onto this one, then?
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'?
Should it match both qualified and simple class names? I believe the Gradle syntax supports that
I can roll out another PR with this implementation, but I'm still going to need the base functionality of |
@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) |
Currently there is no convenient way to execute a single `@ArchTest` rule of a test running with JUnit 4 or 5 support. As a simple mitigation we now provide the possibility to filter a single test via `archunit.properties` / system property. This change allows to e.g. pass ``` -Darchunit.junit.testFilter=rule_field_one,rule_field_two ``` to a test run, which will then filter the tests and only run the `@Archtest` fields or methods with the given name(s). For now we don't support any wildcards or narrowing the match down by surrounding class name to see if this feature is even missed by users. Basic filtering for classes or packages should already be supported by the respective test launcher (i.e. both JUnit 4 and 5 support should respect filter requests for classes or packages already). Issue: #641
When developing large suite of ArchUnit rules, one common pattern is using multiple
ArchRules
/ArchTests
in a single test class file: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*
The text was updated successfully, but these errors were encountered: