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.

Sign up for GitHub

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

Jump to bottom

run tests with JUnit Platform #1294

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

run tests with JUnit Platform #1294

wants to merge 4 commits into from

Conversation

codecholeric
Copy link
Collaborator

To pave the way to migrate to JUnit 5 we now switch the build to use the JUnit Platform runner
by switching to useJUnitPlatform {..} in our Gradle build files.
This doesn't effectively change much on the surface, because we still keep all JUnit 4 annotations and test infrastructure.
But it now runs our JUnit 4 tests using the JUnit Vintage Engine via the Gradle JUnit Platform integration,
instead of directly through the Gradle JUnit 4 integration.
This effectively allows us to combine JUnit 4 and JUnit 5 tests in the same Gradle module
since we can combine any number of JUnit Platform test engines.
We include both JUnit Vintage and JUnit Jupiter engine, so it will pick up JUnit 4 tests as well as JUnit 5 tests.
Then we can migrate tests to JUnit 5 at any pace and any granularity that we want.

@codecholeric
force to run tests of example-junit4 with JUnit 4
eafd697 
We should make sure we always configure `useJUnit {..}`,
even if the property `example` is not set.
Right now this doesn't make a difference,
but when we switch to JUnit 5 this module still needs to use JUnit 4.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric
make out of classpath testing more robust
326daf8 
It has always been a little brittle to use classes
that were compiled regularly and reachable from the
default ClassLoader for test "outside the classpath".
The reason is that if such a class is referenced at any point
before the test runs, then the class is already loaded.
And then we can move these classes out of the folder in the file
system later on all we want, the ClassLoader can still load them
(this was also the reason why we had to use strings for the class names
in such tests instead of referencing the `Class` objects).
I found out that switching to JUnit 5 this actually becomes a problem,
because something in the build infrastructure seems to trigger
a class loading of all classes before already with the test ClassLoader
(maybe it's scanning classes for annotations and by that triggering class loading).

To make the test infrastructure more robust we now don't use regularly
compiled classes out of the URL classpath of the ClassLoader,
but instead we keep the source files as resources and compile them
dynamically when needed.
This way we can be sure that the regular test ClassLoader has no chance
to ever know these classes ahead of being used by a test.
And it also has no chance to load these classes anywhere from the
regular classpath.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric
make IndependentClasspathRule always reset the context ClassLoader
565fa20 
When migrating to JUnit Platform I noticed tests obscurely failing with
symptoms like "ClassNotFoundException" where they worked fine before.
The reason seems to be
```
LocationsTest.locations_of_packages_from_custom_ClassLoader_for_JARs_with_directory_entries
```
Or more specifically the line
```
Thread.currentThread().setContextClassLoader(new URLClassLoader(new URL[]{jarUrl}, null));
```
Since that test doesn't call `independentClasspathRule.configureClasspath()`
the internal `setup` is `null`,
and restoration of the context `ClassLoader` only happens if the `setup` is not null.
Thus, this test pollutes the context `ClassLoader` for tests to come,
which seems to have caused the issues then
(in particular resolving missing classes via `ClassResolverFromClasspath` looks at the
context `ClassLoader` and can then suddenly not load the expected classes).
It seems like when running with JUnit 4 support the context `ClassLoader` was somehow
restored to the `AppClassLoader` which isn't the case anymore with JUnit Platform support.
To make this more foolproof we will now simply always restore the
context `ClassLoader` when `IndependentClasspathRule` is used.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric
run tests with JUnit Platform
105bc09 
To pave the way to migrate to JUnit 5 we now switch the build to use the JUnit Platform runner
by switching to `useJUnitPlatform {..}` in our Gradle build files.
This doesn't effectively change much on the surface, because we still keep all JUnit 4 annotations and test infrastructure.
But it now runs our JUnit 4 tests using the JUnit Vintage Engine via the Gradle JUnit Platform integration,
instead of directly through the Gradle JUnit 4 integration.
This effectively allows us to combine JUnit 4 and JUnit 5 tests in the same Gradle module
since we can combine any number of JUnit Platform test engines.
We include both JUnit Vintage and JUnit Jupiter engine, so it will pick up JUnit 4 tests as well as JUnit 5 tests.
Then we can migrate tests to JUnit 5 at any pace and any granularity that we want.

Note that within `archunit-integration-test` the situation is a little tricky,
because we include both `archunit-junit4` and `archunit-junit5`.
And those share some annotations, but with different effect.
For example, `@AnalyzeClasses` alone triggers the `archunit-junit5-engine` to run a test class.
If it's also annotated with `@RunWith(ArchUnitRunner.class)`, then the JUnit Vintage Engine will pick it up
and run it as a JUnit 4 test.
Thus, I noticed that the tests also annotated with `@RunWith(ArchUnitRunner.class)` are run twice there.
Since this is an irrelevant corner case for real life scenarios, I've simply removed the `@RunWith`,
because we want to run our tests with `archunit-junit5` anyway.

I've compared the Gradle test xml report of all the executed tests before and after.
From what I see the output coincides for both standard and slow tests
(except for one wrongfully executed slow test in `archunit-junit` before the change,
but this is then no problem but instead fixed now).

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric
Copy link
Collaborator Author

I've compared the Gradle test xml report of all the executed tests before and after.
From what I see the output coincides for both standard and slow tests
(except for one wrongfully executed slow test in archunit-junit before the change,
but this is then no problem but instead fixed now).

hankem
hankem approved these changes Jun 1, 2024
View reviewed changes
archunit/src/test/java/com/tngtech/archunit/testutil/OutsideOfClassPathRule.java
Path classFileDir = temporaryFolder.newFolder().toPath();

try (Stream<Path> files = Files.list(sourceFileDir)) {
files.filter(it -> it.getFileName().toString().endsWith(".java"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be

Suggested change
files.filter(it -> it.getFileName().toString().endsWith(".java"))
files.filter(it -> it.toString().endsWith(".java"))

Similarly in copy (if Predicate<String> fileNamePredicate is renamed to pathPredicate).

buildSrc/src/main/groovy/archunit.java-testing-conventions.gradle
dependencies {
testImplementation dependency.junit5JupiterApi

testRuntimeOnly dependency.junit5JupiterEngine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this new dependency for all projects, I was wondering whether archunit-integration-test still needs its testImplementation dependency.junit5JupiterEngine, and I'm actually puzzled whether it even needed it before. 🤔

Similarly, it doesn't seem to need these (introduced with 230a2f4) either:

    testImplementation dependency.mockito
    testImplementation dependency.guava
    testImplementation dependency.log4j_api
    testImplementation dependency.log4j_core
    testImplementation dependency.log4j_slf4j

I've removed them with commit 29f5b92 and tried to consolidate the log4j-dependencies with 6c9e8bb; feel free to cherry-pick whatever you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hankem hankem hankem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@codecholeric @hankem