ArchUnit icon indicating copy to clipboard operation
ArchUnit copied to clipboard

Ability to include/exclude Gradle testFixtures via importOptions

Open britter opened this issue 2 years ago • 10 comments

The com.tngtech.archunit.core.importer.ImportOption class currently only provides ways to include/exclude (among others) tests. With Gradle's java-test-fixtures plugin it's possible to have another source set called testFixtures and a corresponding classes directory containing test fixtures. In my case I want to ignore them from my architecture test, but it's currently not possible. Also there does not seem to be an easy way to extend filters with custom logic.

britter avatar Aug 26 '22 11:08 britter

The good news is that it's actually quite easy to use custom import options: With

class DoNotIncludeTestFixtures implements ImportOption {
    @Override
    public boolean includes(Location location) {
        return !isTestFixture(location);
    }
}

you can use

@AnalyzeClasses(importOptions = DoNotIncludeTestFixtures.class // ...

or

    static final JavaClasses classes = new ClassFileImporter()
            .withImportOption(new DoNotIncludeTestFixtures())
            .import//...

EDIT: Never mind, this was a mistake of mine. There are only good news! 😅 ~The bad news is that the local testFixture source set seems to get compiled to $buildDir/classes/java/test/ just as the test source set, so you cannot distinguish testFixture and test classes unless you manage to change one output directory.~

hankem avatar Aug 26 '22 13:08 hankem

@hankem thank you for the pointer!

The bad news is that the local testFixture source set seems to get compiled to $buildDir/classes/java/test/ just as the test source set, so you cannot distinguish testFixture and test classes unless you manage to change one output directory.

That's not what I'm seeing. If not special configuration is applied, Gradle separates the output directories of source sets. So testFixtures will end up in $buildDir/classes/java/testFixtures.

However while debugging I realized that Gradle will put the test fixture jar on the test runtime classpath, not the source set output folder. So I needed to filter a little bit differently. Here's my full ImportOptions implementation that filters both test classes and test fixtures classes:

public class DotNotIncludeTestsOrTestFixtures implements ImportOption {

    private static final Pattern TESTS_PATTERN = Pattern.compile(".*/build/classes/([^/]+/)?test/.*");

    @Override
    public boolean includes(Location location) {
        return !isTests(location) && !isTestFixtures(location);
    }

    private static boolean isTestFixtures(Location location) {
        return location.isJar() && location.contains("test-fixtures.jar");
    }

    private static boolean isTests(Location location) {
        return location.matches(TESTS_PATTERN);
    }
}

Not sure whether or not this should be a built-in feature. I think it's easy enough to implement. On the other side I don't want to copy this ImportOption into all my projects that use test fixtures.

britter avatar Aug 26 '22 13:08 britter

You might be able to just reuse the existing ImportOption, no? :thinking:

public class DotNotIncludeTestsOrTestFixtures implements ImportOption {
    @Override
    public boolean includes(Location location) {
        return ImportOption.Predefined.DO_NOT_INCLUDE_TESTS.includes(location) && !isTestFixtures(location);
    }

    private static boolean isTestFixtures(Location location) {
        return location.isJar() && location.contains("test-fixtures.jar");
    }
}

codecholeric avatar Aug 26 '22 14:08 codecholeric

That's not what I'm seeing.

Aargh... sorry! 🙈 I didn't notice that my IDE had apparently moved my example class from src/testFixtures/java/ to src/test/java/issue949/ when I tried to refactor the package... 🤦‍♂️

I can now also confirm that src/testFixtures/java gets compiled to $buildDir/classes/java/testFixtures/ (as I hoped initially).

hankem avatar Aug 26 '22 14:08 hankem

About adding it to the built-in ignores, I'm a little torn :thinking: On the one hand, it has been in the official Gradle user guide for a while, so I guess you can call it Gradle standard. On the other hand, this is the first time I hear of anybody actively using it. And additionally there is the problem that if you run the test "with" your IDE (e.g. switch "run tests with" from "Gradle" to "IntelliJ"), then the classpath actually changes and the files will be supplied from the local build dir instead of a JAR file :thinking: So it's also not completely trivial, would add two additional patterns and thus on the other hand slow down the import option for everybody else :thinking:

codecholeric avatar Aug 26 '22 14:08 codecholeric

Since I maybe misunderstood, adding a separate DotNotIncludeTestsOrTestFixtures might be okay, I just wouldn't want to add it to the general DoNotIncludeTests. But then we could also ponder about an ImportOption just DoNotIncludeTestFixtures and then one would declare both importOptions = {DoNotIncludeTests.class, DoNotIncludeTestFixtures.class}. Instead of including the other ImportOption internally :man_shrugging:

codecholeric avatar Aug 28 '22 16:08 codecholeric

On the other hand, this is the first time I hear of anybody actively using it.

Some feature just take some time to be adopted 🙂 We use it quite a bit.

And additionally there is the problem that if you run the test "with" your IDE (e.g. switch "run tests with" from "Gradle" to "IntelliJ"), then the classpath actually changes and the files will be supplied from the local build dir instead of a JAR file 🤔

I wouldn't recommend doing that. It just leads to confusing behavior such as the one you described. I would always delegate to Gradle. This way you get the same results no matter if you run the build from the IDE, CLI or on CI. You can still start tests from your IDE even if you configure it to delegate all build agents to Gradle.

Since I maybe misunderstood, adding a separate DotNotIncludeTestsOrTestFixtures might be okay, I just wouldn't want to add it to the general DoNotIncludeTests. But then we could also ponder about an ImportOption just DoNotIncludeTestFixtures and then one would declare both importOptions = {DoNotIncludeTests.class, DoNotIncludeTestFixtures.class}. Instead of including the other ImportOption internally 🤷‍♂️

I agree, this doesn't belong into DoNotIncludeTests. I think having separate classes makes sense.

britter avatar Aug 29 '22 08:08 britter

I principally agree about always delegating to Gradle, but in practice it's just too slow for my taste :grimacing: I usually delegate the build to Gradle but run tests with IntelliJ as long as I don't run into problems :man_shrugging:

Okay, but then I'll just try do come up with a standard included ImportOption that covers this case :slightly_smiling_face:

codecholeric avatar Sep 09 '22 15:09 codecholeric

@codecholeric so you would be open for a PR that adds the DoNotIncludeTestFixtures import option? I can put that together.

britter avatar Sep 19 '22 07:09 britter

Ah, I already have one in progress :slightly_smiling_face: But thanks a lot for offering! (should probably already have put it there as a draft PR)

codecholeric avatar Sep 19 '22 14:09 codecholeric

Sorry for being so slow with this! It just interfered a little with the 1.0.0 release (+ bug fixes :grimacing:) and I only wanted to consider new features for 1.1.0. You could try the ImportOption from the PR if it works for you, if you want, just to make sure we're merging the right thing :wink: (I have of course tested it locally with a sample project using the Gradle Test Fixtures plugin, but you never know with these different options to put them on the classpath, different Gradle version, etc.)

codecholeric avatar Nov 28 '22 04:11 codecholeric