Skip to content

Conversation

@hankem
Copy link
Member

@hankem hankem commented Mar 31, 2025

This PR upgrades

and for tests:

It reduces the memory footprint of ClassFileImporterSlowTest by exluding classes from sun packages.

@hankem hankem requested a review from codecholeric March 31, 2025 06:24
@hankem
Copy link
Member Author

hankem commented Mar 31, 2025

Maybe not directly related to this PR (also happend for #1334):
ClassFileImporterSlowTest > imports_the_classpath_without_archives failed
with java.lang.OutOfMemoryError: Java heap space for JDK 8 on Ubuntu & MacOS.

@hankem hankem force-pushed the upgrade-dependencies branch 3 times, most recently from 103f090 to 0194182 Compare April 14, 2025 20:54
Copy link
Collaborator

@codecholeric codecholeric left a comment

Choose a reason for hiding this comment

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

Nice, thanks a lot of taking care of the house-keeping 🙂 I have two minor questions I'd be interested about

testImplementation dependency.junit5Jupiter

testRuntimeOnly dependency.junit5VintageEngine
testRuntimeOnly dependency.junitPlatformLauncher
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I fully get this, why exactly does this break in archunit-3rd-party-test? AFAIS that module doesn't even have JUnit Jupiter tests, no? 🤔
Not sure we should really add this globally, even if it solves that symptom, in particular if it's only that one specific test module.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen the

org.junit.platform.commons.JUnitException: OutputDirectoryProvider not available; probably due to unaligned versions of the junit-platform-engine and junit-platform-launcher jars on the classpath/module path.

occur with the upgrade to JUnit 5.12 in other projects, too. I thought it was some kind of version conflict. Do you know what Gradle's useJUnitPlatform actually provides? Some Gradle upgrade notes mention:

If using JUnit 5, an explicit runtimeOnly dependency on junit-platform-launcher is required in addition to the existing implementation dependency on the test engine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, and it turns out that not only archunit-3rd-party-test fails with this JUnitException – that just happened to be the first module where I saw the error. 🙄

Copy link
Member Author

Choose a reason for hiding this comment

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

So I've dropped the misleading reference to archunit-3rd-party-test from the commit message.
If you prefer to understand the change with JUnit 5.12 better, we could also drop that update entirely from this PR, and have it in a separate one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah, if the docs say that you need that platform launcher dependency, then that's good enough for me 😉 Thanks a lot for the explanation!

* Importing those would increase the memory footprint of the test tremendously, but not give an additional benefit for the test,
* so they are excluded for convenience.
*/
private static final ImportOption EXCLUDE_SUN_PACKAGES = l -> !l.contains("/sun/");
Copy link
Collaborator

Choose a reason for hiding this comment

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

What I still don't understand here, what has changed? The rt.jar archive of JDK 8 has been imported for many years, right? Do you know if the memory footprint of ArchUnit was simply smaller a couple of years ago? And if yes by how much? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

In any case, since it always causes problems when we import rt.jar for JDK 8, I'd add this (and the comment) as part of importJavaBaseOrRtAndJUnitJarAndFilesOnTheClasspath() I think 🤔 Then you also wouldn't have to repeat it in every place where that method is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand either where the change comes from, cf. #1446 (comment). I was wondering whether Gradle 8 might execute more tasks in parallel, such that the Github action runners might run out of memory more easily? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd add this (and the comment) as part of importJavaBaseOrRtAndJUnitJarAndFilesOnTheClasspath()

There's also importJavaBase(), but it's certainly a good idea to keep the workaround down with those implementation details. 👍 I'll push an update.

@hankem hankem force-pushed the upgrade-dependencies branch 3 times, most recently from 5e3b0a2 to c8140bb Compare April 22, 2025 17:51
hankem added 6 commits April 27, 2025 16:32
* org.apache.logging.log4j:log4j-{api,core,slf4j2-impl}
  2.24.1 → 2.24.3

* org.assertj:assertj-{core,guava}
  3.26.3 → 3.27.3

Signed-off-by: Manfred Hanke <[email protected]>
  junit-jupiter
= junit-jupiter-api
+ junit-jupiter-params
+ junit-jupiter-engine (runtime)

Signed-off-by: Manfred Hanke <[email protected]>
Without the `junit-platform-launcher` dependency, `./gradlew test` fails with
```
org.junit.platform.commons.JUnitException: TestEngine with ID 'junit-jupiter' failed to discover tests
	at app//org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:160)
	at app//org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverSafely(EngineDiscoveryOrchestrator.java:134)
	at app//org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:108)
	at app//org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:80)
	at app//org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:110)
	at app//org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
	at app//org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:124)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:94)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:63)
	... 16 more
Caused by: org.junit.platform.commons.JUnitException: OutputDirectoryProvider not available; probably due to unaligned versions of the junit-platform-engine and junit-platform-launcher jars on the classpath/module path.
	at app//org.junit.platform.engine.EngineDiscoveryRequest.getOutputDirectoryProvider(EngineDiscoveryRequest.java:94)
	at app//org.junit.jupiter.engine.JupiterTestEngine.discover(JupiterTestEngine.java:67)
	at app//org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:152)
	... 26 more
```

Signed-off-by: Manfred Hanke <[email protected]>
resolves #1446

The CI build has recently often failed on `ubuntu` and `macos` with Java 8,
due to `java.lang.OutOfMemoryError: GC overhead limit exceeded`.

This is due to `ClassFileImporterSlowTest` importing the full classpath in some tests,
which gives more than 20k classes on Java 8,
which contains 7k classes in `com.sun` and 4k classes in `sun` packages
that can easily be ignored.

Signed-off-by: Manfred Hanke <[email protected]>
@codecholeric codecholeric force-pushed the upgrade-dependencies branch from c8140bb to 7739b0b Compare April 27, 2025 14:32
@codecholeric
Copy link
Collaborator

thanks a lot for the update and digging into explanations!

@codecholeric codecholeric merged commit 9b84083 into main Apr 27, 2025
27 checks passed
@codecholeric codecholeric deleted the upgrade-dependencies branch April 27, 2025 15:14
@odrotbohm
Copy link
Contributor

odrotbohm commented May 9, 2025

I hate to be the one asking for this, but could that change be ported back into 1.3? The sole ASM upgrade I mean. 😇

hankem added a commit that referenced this pull request May 10, 2025
…on 69

resolves #1439

Signed-off-by: Manfred Hanke <[email protected]>

(cherry picked from commit a475e03 / #1440)
hankem added a commit that referenced this pull request May 10, 2025
resolves #1455

https://github.com/google/guava/releases/tag/v33.4.4 has
- removed the dependency on JSR-305
  [google/guava@04bf030]
- migrated from Checker Framework annotations to (JSpecify)[https://jspecify.dev/] annotations
  [google/guava@2cc8c5e]
- removed the dependency on Checker Framework annotations
  [google/guava@800b3d4]

compare
- https://repo1.maven.org/maven2/com/google/guava/guava/33.1.0-jre/guava-33.1.0-jre.pom
- https://repo1.maven.org/maven2/com/google/guava/guava/33.4.8-jre/guava-33.4.8-jre.pom

Signed-off-by: Manfred Hanke <[email protected]>

(cherry picked from commit 93e489f / #1440)
@hankem
Copy link
Member Author

hankem commented May 10, 2025

@odrotbohm, the ASM and Guava update has been backported to ArchUnit 1.3.2, which is available on Maven Central.

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

Labels

None yet

Projects

None yet

4 participants