Junit5: Add cyclic dependency checks

[sormuras]

Overview

A simple jdeps junit-jupiter-api-5.3.0-SNAPSHOT.jar results in the output listed below. Fixing the expected output and updating it when introducing new dependencies is a good measure to prevent using internal API and referring to not wanted libraries/modules.

Perhaps a _degraph_-like check can be implemented using this future-proof foundation tool as well. What do you think @schauder ?

junit-jupiter-api-5.3.0-SNAPSHOT.jar -> java.base
junit-jupiter-api-5.3.0-SNAPSHOT.jar -> not found
   org.junit.jupiter.api                              -> java.lang                                          java.base
   org.junit.jupiter.api                              -> java.lang.annotation                               java.base
   ...
   org.junit.jupiter.api                              -> org.opentest4j                                     not found
   org.junit.jupiter.api.condition                    -> java.lang                                          java.base
   ...
   org.junit.jupiter.api.condition                    -> org.junit.platform.commons.util                    not found
   org.junit.jupiter.api.extension                    -> java.lang                                          java.base
   ...
   org.junit.jupiter.api.extension                    -> org.junit.platform.commons.util                    not found
   org.junit.jupiter.api.function                     -> java.lang                                          java.base
   org.junit.jupiter.api.function                     -> org.apiguardian.api                                not found

Deliverables

• [x] Add jdeps integration tests for all published artifacts. Via #1544.
• [x] Remove degraph from build scripts

[jbduncan]

@sormuras I'm a bit lost as to what the output of jdeps junit-jupiter-api-5.3.0-SNAPSHOT.jar is expected to be. Would you kindly clarify things for me?

[sormuras]

For example, the output of jdeps can be checked for cyclic references.

Bad refs found in org.junit.jupiter.engine

• ...descriptor.TestInstanceLifecycleUtils -> ...Constants
• ...discovery.JavaElementsResolver -> ...JupiterTestEngine
• ...execution.ConditionEvaluator -> ...Constants
• ...extension.ExtensionRegistry -> ...Constants

[sormuras]

After #1544 is merged, we should remove degraph from the build scripts:

[sbrannen]

Thanks for investigating all of this, @sormuras!

It's a real shame that Degraph failed to warn us about those package cycles. 😞

[jbduncan]

@sormuras Cool, thanks for clarifying things for me regarding how jdeps would be used!

I'm kinda curious, was ArchUnit considered as an alternative to manually parsing jdeps' output in https://github.com/junit-team/junit5/commit/cf728a4cac330837755f51ae0e4851a8891e9664?

[sormuras]

Does ArchUnit support scanning for cyclic package references? Does it support such a scan for all generated artifacts, i.e. subprojects?

[sormuras]

Seems like it does support checking for cycles: https://www.archunit.org/userguide/html/000_Index.html#_cycle_checks

[schauder]

What package cycles did Degraph miss? I'm afraid I don't understand what the output quoted above means, especially how it is pointing out dependency cycles

[sormuras]

@schauder For example the second line from above:

...discovery.JavaElementsResolver -> ...JupiterTestEngine

describes a cyclic class reference path:

• org.junit.jupiter.engine.JupiterTestEngine uses org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.

• org.junit.jupiter.engine.discovery.JavaElementsResolver refers back to org.junit.jupiter.engine.JupiterTestEngine

[schauder]

Interesting. If anybody cares: This might be because the reference is only a constant, which probably gets inlined, so it isn't present in the bytecode.

[sormuras]

Fixed path above with links to affected lines.

[...] which probably gets inlined, so it isn't present in the bytecode.

Possible. But how does jdeps render those "inlined" edges? Afaik it also scans the bytecode.

[sormuras]

ArchUnit will soon sport its own JUnit Platform TestEngine implementation: https://github.com/TNG/ArchUnit/pull/94

When this is published, we should use it right-away! ;-)

[sbrannen]

Interesting. If anybody cares: This might be because the reference is only a constant, which probably gets inlined, so it isn't present in the bytecode.

That's also quite interesting and enlightening!

So, yeah... references to constants are really so problematic; however, we should still do our best to avoid any such cycles.

[sormuras]

Holding this PR off until we can compare it another one using ArchUnit's TestEngine. Looking forward to the 0.9.0 release, @codecholeric ... ;-)

[codecholeric]

I'll do my best to release this soon :wink: (between today and the weekend I'd guess, just wanna solve one last issue)
However, I have to warn you, ArchUnit also scans bytecode, so it also has the same problems as e.g. Degraph. If a constant gets inlined, the reference is gone from the bytecode and the cycle doesn't exist there anymore and can't be detected.
However this AFAIK only happens with primitives and strings, nevertheless it's a shortcoming (I couldn't find any way to tell the compiler to not do that either, guess the only way is to use a more complex construct to define constants so the compiler isn't smart enough anymore :wink: -> https://stackoverflow.com/questions/3524150/is-it-possible-to-disable-javacs-inlining-of-static-final-variables)

[sormuras]

[...] If a constant gets inlined, the reference is gone from the bytecode and the cycle doesn't exist there anymore and can't be detected. [...]

But how does jdeps report those edges? It is scanning .jar files, i.e. bytecode.

Anyway, it would be totally fine to use this PR and some (other) checks implemented with ArchUnit at the same time.

[codecholeric]

I have no idea how jdeps does that, but I'm open for enlightenment :wink: I just know that in the past I had problems like that with unit tests and a GitHub issue AFAIR as well.
If I look at two classes

public class Foo {
    public static final String yohoo = "yohoo";
}

and

public class Bar {
    void accessConstant() {
        System.out.println(Foo.yohoo);
    }
}

Then the bytecode says

public class com.tngtech.archunit.Bar
Constant pool:
   #1 = Methodref          #7.#18         // java/lang/Object."<init>":()V
   #2 = Fieldref           #19.#20        // java/lang/System.out:Ljava/io/PrintStream;
   #3 = Class              #21            // com/tngtech/archunit/Foo
   #4 = String             #22            // yohoo
...
void accessConstant();
    Code:
      stack=2, locals=1, args_size=1
         0: getstatic     #2                  // Field java/lang/System.out:Ljava/io/PrintStream;
         3: ldc           #4                  // String yohoo
         5: invokevirtual #5                  // Method java/io/PrintStream.println:(Ljava/lang/String;)V
         8: return
...

So there is no Fieldref but the String as a constant and the only reference to Foo seems to be that one entry in the constant pool. Maybe that's how jdeps does it? (no idea why the constant is there though, it's not referenced anywhere...)

[sormuras]

Merged the jdeps-based checks into master.

Keeping this issue open for tracking work on the deliverables listed in the initital description:

• Degraph: remove if superseded by this jdeps`-based cycle check
• ArchUnit: apply more checks, perhaps replace this jdeps`-based cycle check

[codecholeric]

@sormuras It's out now -> https://github.com/TNG/ArchUnit/releases/tag/v0.9.0
If you run into any troubles or are missing anything, make sure to drop me a GitHub issue, gonna fix it ASAP then :wink:
In particular about the JUnit 5 support, don't know if that is bullet proof for all possible environments...