Added support for mock class creation using Byte Buddy #626

raphw · 2016-07-29T13:17:06Z

Adds support for generating classes using Byte Buddy instead of cglib when Byte Buddy is found on the class path. (Other than on AppVeyor, running :spock-specs:test works fine on my machine.)

szpak · 2016-07-29T20:59:31Z

.idea/vcs.xml

@@ -3,6 +3,6 @@
  <component name="VcsDirectoryMappings">
    <mapping directory="" vcs="Git" />
    <mapping directory="$PROJECT_DIR$" vcs="Git" />
+    <mapping directory="$PROJECT_DIR$" vcs="Git" />


Hmm, duplicated entries look strange.

szpak · 2016-07-29T21:03:00Z

Thanks Rafael. I like the way Byte Buddy works in Mockito and I would be happy seeing it in Spock.

+1 for merging it before 1.1.0-final (as it could potentially change the default behavior if Byte Buddy is already on a classpath).

raphw · 2016-07-29T21:12:12Z

Glad to hear. There are some things I would like to mention before merging this:

Byte Buddy can preserve annotations on overridden methods and the super type. This better mimics the original type if any code picks up annotations to interact with an instance. (A cglib mock does not preserve annotations).
It enables default methods on additional interfaces (can be disabled). cglib does not support this
so there can be a change in behaviour if a user does not expect this.
Byte Buddy transparently handles bridge methods. Excluding bridges as done with cglib is generally not a good idea as there are so-called visibility-bridges which should be overridden. This is a change in behaviour.
Byte Buddy handles generics gracefully, e.g. overrides methods appropriately and adds all bridges that the specification demands. It is even possible to extend a generic type (e.g. implement List<String> instead of a raw List.) Byte Buddy would however need to receive such a generic type as an argument.

What is your view on that?

raphw · 2016-07-30T00:19:58Z

On a side note: IntelliJ files should not be version controlled. Those files are considered internal and my installation just picked up the files and changed them without me even noticing. This happens every time I open the project.

szpak · 2016-07-30T23:53:53Z

Byte Buddy can preserve annotations on overridden methods and the super type. This better mimics the original type if any code picks up annotations to interact with an instance. (A cglib mock does not preserve annotations).

Sounds to be useful and quite safe to me.

It enables default methods on additional interfaces (can be disabled). cglib does not support this so there can be a change in behaviour if a user does not expect this.

I think that currently Spock doesn't support additional interfaces in mocks at all.

Byte Buddy transparently handles bridge methods. Excluding bridges as done with cglib is generally not a good idea as there are so-called visibility-bridges which should be overridden. This is a change in behaviour.

I cannot recall any situation when I hit that issue, however, I can image that it would be good to support that. I think that 1.1 already has introduced some incompatibilities and IMHO that one would be acceptable.

Byte Buddy handles generics gracefully, e.g. overrides methods appropriately and adds all bridges that the specification demands. It is even possible to extend a generic type (e.g. implement List instead of a raw List.) Byte Buddy would however need to receive such a generic type as an argument.

There are already some issues with mocking generic types in Spock. I would need to dig into to say how does it look.

However, there is one issue that came to my mind. Currently all internal tests in Spock have been switched to ByteBuddy (as it is on a classpath) and we lost regression testing against cglib (in Mockito cglib support was dropped, so it was not a problem). I don't know how much feasible is to already have Byte Buddy on a classpath (as a 3rd party dependency?) in the real project and still want to use cglib in Spock, but in a case of regression people may want to be able to switch to cglib for particular mocks (like additional configuration parameter in IMockConfiguration).

I don't have a good idea how to provide testing for both ByteBuddy and cglib without using some global configuration parameter (like a system property) set in an additional Gradle test task (for cglib). However, maybe we don't need it or maybe we may even want to treat cglib support as the 2nd class citizen (aka "deprecated mode")?

@robfletcher @sbglasius @leonard84 @Fuud WDYT?

robfletcher · 2016-08-02T18:08:13Z

Can we get an example of the bridge method issue? I don't quite follow what we're losing or gaining there.

It would be a very good idea to have some kind of testing in place for cglib unless we are dropping support for it entirely.

raphw · 2016-08-04T08:26:09Z

The problem with bridge methods is that they are much more complex than they might appear. For this reason, Byte Buddy does not expose bridge methods or requires them to be handled manually in any context. For example, with cglib, you simply exclude any method with the bridge modifier. This can cause trouble in scenarios like:

/* package-private*/ class Foo { 
  public String foo() { return "foo"; } 
}
public class Bar extends Foo { }

The Java compiler will add a visibility-bridge for Bar to promote foo to become factually public (as no method within a package-private type is treated as public by core reflection) and compile the following class:

public class Bar extends Foo { 
  public /* synthetic bridge */ String foo() { return super.foo(); } 
}

By excluding bridge methods from interception, you do no longer mock the foo method with your cglib exclusion. This is typically a problem when working with inner/nested classes which often are compiled into a package-private type.

Also, cglib does not implement reified methods for a mocked type what can cause problems or change in behavior with some libraries and tools. So to speak, if you are mocking some:

public class Foo<T> {
  public void foo(T val) { } 
}
public class Bar extends Foo<String> { }

Byte Buddy will add the correct bridge when instrumenting Bar just as any compiler would do when extending Bar and overriding foo.

public class Bar$SpockMock extends Bar {
  public void foo(String val) { }
  public /* synthetic bridge */ void foo(Object val) { foo((String) val); }
}

Note that some frameworks will expect to being able to look up the actual method via a call similar to type.getMethod("foo", String.class) where the cglib mock would fail as it only overrides the Object signature method and does not implement the factual signature as required by the JLS.

As for testing cglib: Maybe Spock can supply a system property that decides if Byte Buddy is considered. This would allow both to (a) retain old testing behavior even if Byte Buddy is already on the class path (b) to run a build with mocks created by cglib. As an alternative, in Mockito, this is solved by using plugins where a build can register a custom byte code provider.

leonard84 · 2016-08-07T15:54:02Z

spock-core/src/main/java/org/spockframework/mock/runtime/ProxyBasedMockFactory.java

+      Class<?> enhancedType = CACHE.find(classLoader, type, additionalInterfaces);
+
+      if (enhancedType == null) {
+        synchronized (type) {


Isn't this problematic, if someone else decides to lock based on a class. Why not lock on an internal object or use a real lock?

The implicit cglib cache locks on the class object already today. This is just a recreation.

You are right that this can cause problems if the class object is already locked by another thread. Using a single monitor for this would however not allow concurrently creating mocks for different classes from different threads. The question is, what limitation is preferred.

Yeah your are right, both approaches have their drawbacks. Could you document the rationale for this decision in a comment.

Typically, people create a single mock type per class. The alternative where Byte Buddy would need to create multiple mock types for mocking a type plus additional (explicitly specified) interfaces is rather the exception. Given that most mock types derive of a single type, the application is lock-free for these scenarios. Covering the 99% scenario lock free is probably a sane approach. Also, this requires locking only a single monitor such that dead locks are impossible (live locks are however still possible).

Locking on the class loader is more likely to cause problems, though, as class loaders typically lock on themselves during class loading. Creating a mock can cause class loading as using the reflection API (for finding methods to override) causes eager resolution of a type where all types references in methods are loaded. This is typically not a problem but can be one with parallel capable class loaders.

Thanks for the explanation, but I meant a short source code comment why we lock on class an not on an internal lock. Just to document that this is intentional and why.

leonard84 · 2016-08-07T16:06:20Z

@szpak valid points

but in a case of regression people may want to be able to switch to cglib for particular mocks (like additional configuration parameter in IMockConfiguration).

IMHO mixing implementations would make it more complicated.

I don't have a good idea how to provide testing for both ByteBuddy and cglib without using some global configuration parameter (like a system property) set in an additional Gradle test task (for cglib). However, maybe we don't need it or maybe we may even want to treat cglib support as the 2nd class citizen (aka "deprecated mode")?

As long as we support it we need to test it. As for how, I think it should be possible with gradle to reuse the test sources and have two test projects with different dependency sets. Or maybe even in the same project using two test configurations (I need to refresh my gradle-fu).

leonard84 · 2016-08-07T16:11:25Z

spock-core/src/main/java/org/spockframework/mock/runtime/ProxyBasedMockFactory.java

    } else if (cglibAvailable) {
      proxy = CglibMockFactory.createMock(mockType, additionalInterfaces,
          constructorArgs, mockInterceptor, classLoader, useObjenesis);
    } else {
      throw new CannotCreateMockException(mockType,
-          ". Mocking of non-interface types requires the CGLIB library. Please put cglib-nodep-2.2 or higher on the class path."
+          ". Mocking of non-interface types requires the CGLIB library. Please put Byte Buddy (at least 1.4.0) or cglib-nodep-2.2 or higher on the class path."


Not really your issue but if you are already touching it. The message should say that we need at least version 3.2 of cglib.

raphw · 2016-08-19T11:53:58Z

As an additional incentive to merge this: Byte Buddy supports Java 9 by now, cglib does not (yet?). ;)

leonard84 · 2016-08-19T12:32:31Z

Yes, as soon as the testing issue is resolved. We need tests to run against both cglib and ByteBuddy.

raphw · 2016-08-23T12:29:50Z

I added a property -Dorg.spockframework.mock.ignoreByteBuddy=true which can be set to use cglib if both Byte Buddy and cglib are available. This can also be used by end-users if required. Alternatively, it would be possible to simply rerun the tests without Byte Buddy on the class path.

I am not a Gradle person. Were you waiting for me to add support for testing both? I would need to read up my Gradle 101 to do this.

szpak · 2016-08-23T19:36:35Z

Learning Gradle could be beneficial for you, @raphw ;)

However, if it's not the first point on your TODO list I would be probably able to get it a try once finishing my holiday season (probably somewhere at the edge of September and October ;) ). Luckily, someone else will be able to handle it earlier.

raphw · 2016-09-01T09:00:09Z

@szpak @leonard84 I recently had the pleasure of writing a Byte Buddy plugin for Gradle and got to know the build system a bit better. I went for a simple solution where running Gradle with:

gradle test -DuseCglib=true

runs all tests with cglib rather then Byte Buddy. From the comments in your .travis.yml, it does however seem like you do not want additional builds.

Furthermore, AppVeyor does not seem to even attempt building the project. Neither do I understand why Travis does no longer build the branch.

leonard84 · 2016-09-01T13:04:27Z

@raphw my approach would be to define another test task, e.g., testCglib which is then added to the ci-builds in the main gradle file.

raphw · 2016-09-01T13:56:50Z

@leonard84 Would you know how to do that? I googled a bit, but there seems no easy way of duplicating a task in Gradle.

leonard84 · 2016-09-01T14:11:40Z

If you look at https://www.petrikainulainen.net/programming/gradle/getting-started-with-gradle-integration-testing/ you can see how to define another test task. In your case I think it would suffice to just set the SystemProperty and maybe use another output directory for the reports.

raphw · 2016-09-06T08:17:25Z

@leonard84 Thanks for the link. I followed the recommendation and have a testCglib task integrated in the build.

szpak · 2016-09-06T10:59:03Z

Thanks @raphw!

LGTM

leonard84 · 2016-09-09T08:30:53Z

thanks @raphw you did all what is neccessary as far as I can see, the reason why we haven't merged yet is that it now doubles our test-time which nearly double the total build time.
Maybe we can tweak the testCglib task a bit to only include tests which really use class-mocks and only run on CI-builds by default.

raphw · 2016-09-09T09:56:46Z

It would be possible to filter out some tests but then again, code generation is a very complex process that is hard to test in isolation. Maybe, the tests should only run on CI but not on a standard build?

raphw · 2016-09-15T10:25:20Z

I changed the build now to only attach the testCglib task to the check task if it is run on a CI server. Otherwise, it is still possible to run the task manually on a local machine, if required.

Would this suffice?

robfletcher · 2016-09-23T21:38:15Z

I'm for merging this… any objections?

raphw · 2016-09-30T09:08:07Z

Ping @leonard84 @szpak - What is the state on this?

szpak · 2016-09-30T22:48:47Z

I've already approved your changes in a review so I cannot be against :)

szpak reviewed Jul 29, 2016
View reviewed changes

szpak mentioned this pull request Jul 29, 2016

PollingConditionsSpec.provides fine-grained control over polling fails on AppVeyor #627

Open

leonard84 reviewed Aug 7, 2016
View reviewed changes

szpak approved these changes Sep 23, 2016

View reviewed changes

Added support for mock class creation using Byte Buddy.

214d566

robfletcher merged commit d96f3ac into spockframework:master Sep 30, 2016

leonard84 mentioned this pull request Feb 21, 2017

Spock fails to intercept certain bridge methods #698

Closed

jochenberger mentioned this pull request Feb 23, 2017

support for the new @ImportModule annotation #78

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for mock class creation using Byte Buddy #626

Added support for mock class creation using Byte Buddy #626

raphw commented Jul 29, 2016 •

edited

szpak Jul 29, 2016

szpak commented Jul 29, 2016 •

edited

raphw commented Jul 29, 2016 •

edited

raphw commented Jul 30, 2016 •

edited

szpak commented Jul 30, 2016

robfletcher commented Aug 2, 2016

raphw commented Aug 4, 2016 •

edited

leonard84 Aug 7, 2016

raphw Aug 7, 2016 •

edited

leonard84 Aug 15, 2016

raphw Aug 15, 2016

leonard84 Aug 15, 2016

leonard84 commented Aug 7, 2016

leonard84 Aug 7, 2016

raphw Aug 15, 2016

raphw commented Aug 19, 2016 •

edited

leonard84 commented Aug 19, 2016

raphw commented Aug 23, 2016 •

edited

szpak commented Aug 23, 2016

raphw commented Sep 1, 2016 •

edited

leonard84 commented Sep 1, 2016

raphw commented Sep 1, 2016

leonard84 commented Sep 1, 2016

raphw commented Sep 6, 2016 •

edited

szpak commented Sep 6, 2016

leonard84 commented Sep 9, 2016

raphw commented Sep 9, 2016 •

edited

raphw commented Sep 15, 2016

robfletcher commented Sep 23, 2016

raphw commented Sep 30, 2016

szpak commented Sep 30, 2016

Added support for mock class creation using Byte Buddy #626

Added support for mock class creation using Byte Buddy #626

Conversation

raphw commented Jul 29, 2016 • edited

szpak Jul 29, 2016

Choose a reason for hiding this comment

szpak commented Jul 29, 2016 • edited

raphw commented Jul 29, 2016 • edited

raphw commented Jul 30, 2016 • edited

szpak commented Jul 30, 2016

robfletcher commented Aug 2, 2016

raphw commented Aug 4, 2016 • edited

leonard84 Aug 7, 2016

Choose a reason for hiding this comment

raphw Aug 7, 2016 • edited

Choose a reason for hiding this comment

leonard84 Aug 15, 2016

Choose a reason for hiding this comment

raphw Aug 15, 2016

Choose a reason for hiding this comment

leonard84 Aug 15, 2016

Choose a reason for hiding this comment

leonard84 commented Aug 7, 2016

leonard84 Aug 7, 2016

Choose a reason for hiding this comment

raphw Aug 15, 2016

Choose a reason for hiding this comment

raphw commented Aug 19, 2016 • edited

leonard84 commented Aug 19, 2016

raphw commented Aug 23, 2016 • edited

szpak commented Aug 23, 2016

raphw commented Sep 1, 2016 • edited

leonard84 commented Sep 1, 2016

raphw commented Sep 1, 2016

leonard84 commented Sep 1, 2016

raphw commented Sep 6, 2016 • edited

szpak commented Sep 6, 2016

leonard84 commented Sep 9, 2016

raphw commented Sep 9, 2016 • edited

raphw commented Sep 15, 2016

robfletcher commented Sep 23, 2016

raphw commented Sep 30, 2016

szpak commented Sep 30, 2016

raphw commented Jul 29, 2016 •

edited

szpak commented Jul 29, 2016 •

edited

raphw commented Jul 29, 2016 •

edited

raphw commented Jul 30, 2016 •

edited

raphw commented Aug 4, 2016 •

edited

raphw Aug 7, 2016 •

edited

raphw commented Aug 19, 2016 •

edited

raphw commented Aug 23, 2016 •

edited

raphw commented Sep 1, 2016 •

edited

raphw commented Sep 6, 2016 •

edited

raphw commented Sep 9, 2016 •

edited