junit-team / junit5

✅ The 5th major version of the programmer-friendly testing framework for Java and the JVM
https://junit.org
Eclipse Public License 2.0
6.43k stars 1.49k forks source link

Provide a MethodOrderer that uses the order in the source file #1919

Closed t1 closed 3 years ago

t1 commented 5 years ago

The MethodOrderer implementation OrderAnnotation uses an @Order annotation to allow sorting the execution order of tests (which is generally a smell, but sometimes necessary). But that means another annotation for every test method. It would be nicer to simply use the order of the methods in the source. As this information is not provided by the Java reflection API, we'd have to parse the class file. Maybe we don't need a full parser, but a simple heuristic may be enough.

Some years ago I already wrote a simple JUnit 4 runner for this here

Deliverables

t1 commented 5 years ago

Scanning class files might take some time, but as ordering test execution is generally only useful for integration tests, it should be negligible, shouldn't it?

sbrannen commented 5 years ago

Thanks for raising the issue.

I actually already had the same idea, though I was considering using ASM to read the byte code, in which case we would shadow the ASM dependency.

I've tentatively slated this for the 5.6 backlog for team discussion.

In any case, feel free to submit a PR with your handcrafted byte code reader if you like.

sormuras commented 5 years ago

Wouldn't we have to upgrade the shadowed ASM version every 6 month to be able to scan the latest binaries produced by the latest JDK? /cc @forax

sbrannen commented 5 years ago

Wouldn't we have to upgrade the shadowed ASM version every 6 month to be able to scan the latest binaries produced by the latest JDK? /cc @forax

Mmmm.... yes, that is a potential downside of depending on an external framework, especially with the new JDK release cadence.

So, if we can use something handcrafted (that works reliably), that would naturally be a better fit.

t1 commented 5 years ago

The heuristic I used had the downside that it was confused by fields with the same name as a method, but this should rarely be an issue, shouldn't it?

I will start to work on a first PR.

sbrannen commented 5 years ago

The heuristic I used had the downside that it was confused by fields with the same name as a method, but this should rarely be an issue, shouldn't it?

I honestly don't know. It's hard to make a call on something like that, and we should try to avoid such shortcomings if possible.

I will start to work on a first PR.

OK. Thanks.

t1 commented 5 years ago

Just created a draft PR.

Who is supposed to check the DoD? All but the user guide and release notes should be done.

t1 commented 5 years ago

Just added to the user guide. I can add to the release notes as soon as it's scheduled.

sbrannen commented 5 years ago

Just created a draft PR.

Thanks. We'll check it out.

Who is supposed to check the DoD?

You can check off what you think you have accomplished, and we'll review the points before merging if we choose to accept the PR.

forax commented 5 years ago

It will be a PITA to maintain such feature because the order in the .class file is different from the order in the source code.

As far as i know, javac, ecj, kotlinc, clojure do not provide such guarantee, worst, the bytecode can be transformed by an agent at runtime, and most agents tend remove methods and add new ones at the end because it's more efficient than changing the bytecode of a method at runtime.

sbrannen commented 5 years ago

Well, the byte code typically contains the line numbers (unless the code was explicitly compiled with the flag to turn that off).

So, for example, if we were to look up the value of [pc: 0, line: ???] under Line numbers: in the byte code for each method, the ??? would be the actual line number from the source code, and we could then sort based on that value.

And that should be reliable -- right?

t1 commented 5 years ago

AFAIK there is no guarantee, but it actually does work. The scrambling of the method order happens at runtime in the Class class. And even if the heuristic is broken in a future version of Java, it would be possible to fully parse the bytecode.

t1 commented 5 years ago

Any update to this?

marcphilipp commented 5 years ago

I just took a look at your PR for this (#1922). It does "class-file order". Have you considered reading the line numbers (if present) from the bytecode as suggested by @sbrannen above?

marcphilipp commented 3 years ago

Team decision: Since there's no official API to query this, the implementation would have to rely on parsing class files which we don't feel belongs in JUnit proper.

LifeIsStrange commented 3 years ago

@t1 @marcphilipp could you make this an optional extension dependency? It would be so much useful, even if support would break in future JVM versions (seems highly unlikely) I guess it would always be fixable by slightly changing the parsing strategy. Hence it make sense for many to use this opt-in, being warned of this hypothetical sensitivity to JVM version in the Readme. If such dependency was under junit-team organization that would allow to make it more widely known.

forax commented 3 years ago

even if support would break in future JVM versions (seems highly unlikely)

you mean likely :)

Part of the OpenJDK project Valhalla is about specializing generics, the current draft proposes to group methods by type parameters (Specialization_Anchor) so a possible side effect is the order of the methods inside a classfile.

And what about when the bytecode is not being available like with Graal Native Image and the OpenJDK project Leyden ?

[1] http://cr.openjdk.java.net/~jrose/values/parametric-vm.pdf

jbduncan commented 3 years ago

Welp.

The only other way forward I can think of is to use a Java parsing library to parse the tests' source code, but I'm pretty sure won't work, because JUnit needs the tests' compiled form - classfiles in JARs - to run them, IIUC.

Not to mention that asking users to also provide Java source files would be painful, and JUnit would have to be turned into a pseudo-compiler/build tool to link up the source files with the classfiles, I imagine. :confounded:

And this is ignoring that Kotlin is a popular language for writing JUnit tests nowadays, and I'm sure that people use other JVM languages with JUnit too.

tl;dr: Even just parsing test source code would be too much work.

marcphilipp commented 3 years ago

@LifeIsStrange MethodOrderer is completely extensible so this could be developed as an independent third-party extension or as part of JUnit Pioneer.

LifeIsStrange commented 2 years ago

is this work related? https://github.com/openjdk/jdk/commit/24e586a04368a76cd9f37aa783b974b9e0351d58

sormuras commented 2 years ago

is this work related? https://github.com/openjdk/jdk/commit/24e586a04368a76cd9f37aa783b974b9e0351d58

No.

https://bugs.openjdk.java.net/browse/JDK-8276764 is about keeping order within a "class file container", such as in a .jar or .jmod file.

LifeIsStrange commented 2 years ago

Edit: ignore my comment, I was wrong

https://github.com/spring-projects/spring-framework/issues/27701

Spring 6.0 now expose a method getDeclaredMethods that gets the methods in a class in their order of declaration. Since it is used internally we can assume the approach is reliable and actively maintained. I wonder how Junit could leverage this implementation.

forax commented 2 years ago

@LifeIsStrange The order of the methods inside the classfile is different from the order of the methods in the Java file, see [1].

What Spring does is maintaining the same order of the methods in the classfile when the bytecode is transformed, so this is unrelated to this issue.

[1] https://github.com/spring-projects/spring-data-commons/blob/788457c90132ae7ca893791d091279faa8e76abe/src/main/java/org/springframework/data/type/MethodsMetadata.java#L42

LifeIsStrange commented 2 years ago

@forax

Your point apply to getMethods() and you are very probably right but it is unclear to me wether this also apply to the new method getDeclaredMethods() Its documentation says:

Retrieve the method metadata for all user-declared methods on the underlying class, preserving declaration order as far as possible.

According to its documentation it seems to preserve user supplied declaration order.

https://github.com/spring-projects/spring-framework/commit/50c7c848860fea9ae1ba8d9ce12a9ee7e2eee45f @jhoeller Feel free to ignore my comment regarding the doc comment since I'm probably just misunderstanding and being wrong

sbrannen commented 2 years ago

Retrieve the method metadata for all user-declared methods on the underlying class, preserving declaration order as far as possible.

According to its documentation it seems to preserve user supplied declaration order.

That documentation in Spring might be a little misleading. The interface in question has two implementations: one using reflection and one using ASM to analyze the byte code.

For the reflection-based implementation, the declaration order most likely will not (and potentially never will) represent the order in the source code.

For the ASM-based implementation, the declaration order may represent the order in the source code, but I don't know if there is any guarantee for that.

It may be the case that org.springframework.asm.ClassVisitor.visitMethod(int, String, String, String, String[]) (which is a shadowed version of the same, official API from ASM) gets invoked in the order in which methods are declared in the source code, but the Javadoc from ASM doesn't mention that.

Of course, @jhoeller, feel free to correct me if I'm wrong.

class101 commented 2 years ago

Thank you for the link @sbrannen To note fore other readers, I provide a working sample by annotating a test class

@TestMethodOrder(TestMethodOrderer.SourceCode.class)

credits : Warren MacEvoy @wmacevoy

LifeIsStrange commented 2 years ago

Thanks for sharing!

class101 commented 2 years ago

Thanks for sharing!

You are welcome @LifeIsStrange

In case this is of interest, I have updated my code today to support the following case when you want to order Classes + Method in source code order like the following sample :

When you have multiple standalone test classes that you want to maintain standalone, but want to offer the possibility to execute ALL tests in a specific order, you just need to extends your test classes in a similar AllSpringTests parent class, mark the classes @Nested and extends your real tests, then you mark each @Nested classe with @TestMethodOrder(TestMethodOrderer.SourceCode.class)

@TestClassOrder(ClassOrderer.OrderAnnotation.class)
class AllSpringTests
{
  @Order(1)
  @Nested
  @TestMethodOrder(TestMethodOrderer.SourceCode.class)
  class NestedSpringTests1 extends SpringTests1
  {
  }

  @Order(2)
  @Nested
  @TestMethodOrder(TestMethodOrderer.SourceCode.class)
  class NestedSpringTests2 extends SpringTests2
  {
  }

  @Order(3)
  @Nested
  @TestMethodOrder(TestMethodOrderer.SourceCode.class)
  class NestedSpringTests3 extends SpringTests3
  {
  }
}

Doing so you will need to update my code

    @Override
    public void orderMethods(MethodOrdererContext context)
    {
      MethodDescriptorOrder.methodsInSourceCodeOrder = Reflect.getDeclaredMethodsInOrder(context.getTestClass());
      context.getMethodDescriptors().sort(sourceCodeComparator);
    }

to

    @Override
    public void orderMethods(MethodOrdererContext context)
    {
      //MethodDescriptorOrder.methodsInSourceCodeOrder = Reflect.getDeclaredMethodsInOrder(context.getTestClass());
      //context.getMethodDescriptors().sort(sourceCodeComparator);

      /* Nested classes support */
      List<? extends MethodDescriptor> methods = context.getMethodDescriptors();
      Class<?> testClass = ((DefaultMethodDescriptor) methods.get(0)).getAnnotatedElement().getDeclaringClass();
      MethodDescriptorOrder.methodsInSourceCodeOrder = Reflect.getDeclaredMethodsInOrder(testClass);
      context.getMethodDescriptors().sort(sourceCodeComparator);
    }
LifeIsStrange commented 2 years ago

Thanks, I will test this for my next spring project!

class101 commented 1 year ago

@LifeIsStrange

To note that the class can be cleaned up and enhanced. We use here an updated version we never republished but if any interested ping me, the original is full of reflective and unnecessary imports,in reality reading only byte code of the class is necessary

t1 commented 1 year ago

Since there's no official API to query this, the implementation would have to rely on parsing class files which we don't feel belongs in JUnit proper.

With JEP 457, we will be getting an official JDK-API to do this. have... to... be... patient 🤪

class101 commented 1 year ago

It should be supported by junit since a long time ago. Sorting out how test are executed is only JUnit job not the JDK job