Skip to content
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

Ability to include/exclude Gradle testFixtures via importOptions #949

Closed
britter opened this issue Aug 26, 2022 · 11 comments · Fixed by #1010
Closed

Ability to include/exclude Gradle testFixtures via importOptions #949

britter opened this issue Aug 26, 2022 · 11 comments · Fixed by #1010

Comments

@britter
Copy link

britter commented Aug 26, 2022

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.

@hankem
Copy link
Member

hankem commented Aug 26, 2022

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.

@britter
Copy link
Author

britter commented Aug 26, 2022

@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.

@codecholeric
Copy link
Collaborator

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

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");
    }
}

@hankem
Copy link
Member

hankem commented Aug 26, 2022

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).

@codecholeric
Copy link
Collaborator

About adding it to the built-in ignores, I'm a little torn 🤔 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 🤔 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 🤔

@codecholeric
Copy link
Collaborator

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 🤷‍♂️

@britter
Copy link
Author

britter commented Aug 29, 2022

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.

@codecholeric
Copy link
Collaborator

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

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

@britter
Copy link
Author

britter commented Sep 19, 2022

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

@codecholeric
Copy link
Collaborator

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

@codecholeric
Copy link
Collaborator

Sorry for being so slow with this! It just interfered a little with the 1.0.0 release (+ bug fixes 😬) 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 😉 (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.)

hankem added a commit that referenced this issue Dec 2, 2022
Gradle provides the `java-test-fixtures` plugin out of the box, which
will add a separate source set for creating test fixtures (i.e. test
support code). This source set can see the main sources and be seen by
the test sources. Depending on how the tests are run they might be put
onto the classpath as folder path (containing the compiled sources) or
JAR archive. Thus, we have to exclude both, the folder where Gradle
compiles the files to by convention and archives that follow the Gradle
convention to end in `-test-fixtures.jar`. However, this can of course
only be a heuristic. Custom modules that mimic the convention will e.g.
also be excluded. But in these cases users have to customize their
import options then, instead of using the predefined one.

Resolves: #949
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants