Use byte buddy instead of cglib

[filiphr]

Check List:

• Related to: Java 9 reflection changes #928
• Unit tests : NA
• Javadoc with a code example (API only) : NA

With this PR I am dropping cglib and adding bytebuddy as a dependency. I still have not removed the shading. One test is failing due to ClassCastException, and I am not entirely sure how I can resolve it. Maybe @raphw can help out a bit 😄. The issue is in the ErrorCollector#intercept method.

I think that we should not be shading bytebuddy. Their recommendation is that you should shade if you use the exposed ASM, which I am not. Also the other major players such as Mockito are pulling it as a dependency (we could do it as an optional one or provide 2 modules, one with normal assertions and another with soft).

Something that I noticed with bytebuddy is that you can theoretically generate the bytecode and ship that as part of the AssertJ release (that way you have no dependency on anything during runtime)

[joel-costigliola]

@filiphr I'm not sure I fully understand everything, does that mean soft assertions will never be able to support methods that change the return type?

[joel-costigliola]

@filiphr I pushed some commits on the PR branch after rebasing it to master, the failing tests concerns only assumptions with stream, I haven't figured out why but it seems the assumption proxy mechanism is ignored.

[joel-costigliola]

I finally figured out why some methods are ignored, it's because they are final to be able to use @SafeVarargs so that users don't have a warning when using such methods.

@raphw in that case, can you confirm that the way to go with byte-buddy is using an agent as you suggested in raphw/byte-buddy#26 ?

[raphw]

Yes, the only way to proxy a final method is by using a Java agent to redefine the actual code. You can use Byte Buddy's advice for doing so and the byte-buddy-agent project for attaching an agent to the current process. Based on this agent, you can redefine an existing method. If you want to go for this solution, I can assist you with details.

Fair warning: attaching an agent is an expensive procedure (1-2 seconds and 3-5 in Java 9 as it requires starting a helper process). Especially users that run simple tests often do often not appreciate this expensive one time cost.

Unfortunately, there is no other solution on the JVM. Also, this approach does not work with final native methods.

[joel-costigliola]

Thank you so much for your help, really appreciated.

I'm reluctant to use the agent because of the performance penalty. I think I'll try to add subclasses like SoftAssertionListAssert but for assumptions, this is a class that does not override methods as final to put some @SafeVarargs so it should ok to use it for proxying.

Another approach I'm keen to evaluate for assumptions is to generate the assumption wrappers code, assumptions mechanism being stateless I believe it is a good candidate, soft assertions is a bit more complex in that regard and I'm not sure it is possible/handy to use code generation for that.

Anywya we need to be better at testing assumptions and soft assertions, we have missed a few bugs.

[joel-costigliola]

I fixed the tests in 237257b but that uncovered areas that were not tested, for example soft assertions on streams, ex:

  @Test
  public void should_work_with_stream() {
    try {
      softly.then(Stream.of("a", "b", "c")).contains("a", "b")
            .contains("d")
            .isEmpty();
      softly.assertAll();
    } catch (SoftAssertionError e) {
      List<String> errors = e.getErrors();
      assertThat(errors).hasSize(2);
    }
  }

this succeeds on master but fails in this branch for the same reason as before, i.e. contains is final.
to make it work, then(Stream actual) implementation must proxy StreamAwareListAssert instead of ListAssert.

[raphw]

One way to get around the final method issue is to write adapters that delegate to some protected method that can still be proxied:

interface Foo<T> {
  void bar(T... t);
}

abstract class FooAdapter<T> implements Foo<T> {
  @SafeVarArgs
  public final void bar(T... t) {
    doBar(t);
  }
  protected abstract void doBar(T[] t);
}

Would that be an option?

[joel-costigliola]

@raphw yes that's a good option too.

[joel-costigliola]

Done, thanks @filiphr and @raphw!

[filiphr]

No problem @joel-costigliola, I am really glad that we got this in. Thank you for picking it up abd finishing it.

[sormuras]

Looking forward to 3.9.1. ✔️