Feature/issue 641 filtering env props

[crizzis]

[codecholeric]

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]

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

[codecholeric]

| 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 :slightly_smiling_face: Also bugs we should fix ASAP without waiting for a full feature to be done...

[crizzis]

| 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]

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.