Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide interfaces for all OpenTest4J exceptions #55

Open
kriegfrj opened this issue Mar 8, 2019 · 4 comments
Open

Provide interfaces for all OpenTest4J exceptions #55

kriegfrj opened this issue Mar 8, 2019 · 4 comments

Comments

@kriegfrj
Copy link

kriegfrj commented Mar 8, 2019

This is a generalisation of #27. It flows out of some work I've been trying to do to add OpenTest4J support to Mockito (mockito/mockito#1649).

In my (admittedly brief) experience, the main issue holding back migration of assertion frameworks to OpenTest4J is being able to simultaneously maintain:

  1. Loose coupling to OpenTest4J, and
  2. Backward compatibility.

It is possible to achieve 100% of one or the other in isolation. However, it is not possible to simultaneously achieve both 100%.

Take Mockito for example. They already have two versions of ArgumentsAreDifferent (in different packages). There are tests out there in the wild which will try to catch ArgumentsAreDifferent. You have two options for going forward:

  1. Change ArgumentsAreDifferent so that it inherits from org.opentest4j.AssertionFailedError. This preserves backwards compatibility, but creates a hard dependency on OpenTest4J.
  2. Create a new version of ArgumentsAreDifferent (with a different class name or package) that inherits from org.opentest4j.AssertionFailedError. This can be configured in a way that the new version is only loaded/used if OpenTest4J is on the classpath at runtime (thus preserving loose coupling), but it breaks backward compatibility because this new assertion won't be caught by the existing tests.

I think that the way around this is to have a set of parallel interfaces that define the methods of the various OpenTest4J assertion classes. This introduces a third possibility to the above problem. Thus, supposing (for example) that the parallel interface for AssertionFailedError is IAssertionFailedError, we have the following option:

  1. Create a new version of ArgumentsAreDifferent that inherits from the existing version of ArgumentsAreDifferent and implements IAssertionFailedError. Maybe someone who knows more about how proxying works in Java could provide a proxy factory utility in OpenTest4J that then allows (eg) Mockito to easily implement all the methods of IAssertionFailedError on their own implementation by proxying through to the reference implementation. In this way, backwards compatibility and loose coupling are both preserved.

Such a change would require getting the IDEs on board for some minor changes. Instead of looking for AssertionFailedError, they would need to look for instances of Throwable (or subclasses) and see if they implement IAssertionFailedError, and cast & handle as appropriate. But hopefully, it will not be too hard to get the IDEs on-board.

This could be seen as a transitional approach. If/when the major assertion libraries are ready to move to a hard dependency on OpenTest4J, it could be deprecated. I think if this migration path were offered, it could speed up the adoption of OpenTest4J.

Thoughts?

@marcphilipp
Copy link
Member

Interesting idea!

@sbrannen @sormuras @mmerdes WDYT?

@kriegfrj
Copy link
Author

kriegfrj commented Mar 9, 2019

Maybe someone who knows more about how proxying works in Java could provide a proxy factory utility in OpenTest4J that then allows (eg) Mockito to easily implement all the methrods of IAssertionFailedError on their own implementation by proxying through to the reference implementation.

It occurred to me that with Java 8 (which allows default implementations for interface methods), this proxy trick would be unnecessary. Simply provide default implementations of all the methods. This would require setting Java 8 as the minimum target though, and not everyone's there yet...

@sbrannen
Copy link
Member

I think it's an intriguing idea.

However, I don't think proxy generation is a good solution for this.

  • JDK dynamic proxies only work with interfaces and are therefore not an option.
  • Any form of dynamic subclassing for proxy generation requires a third-party library such as CGLIB, ByteBuddy, etc., and I think OpenTest4J should not be pulling in third-party dependencies (unless shaded, but even debatable then).
  • Dynamic subclassing imposes restrictions on the proxied types -- for example, they cannot be private, final, or have private/final methods (that need to be proxied). Constructors can also be problematic.
  • Any form of proxying will result in runtime overhead.
  • The proxy generation implementation would have to be maintained which can become a burden over time due to changes in the JDK, etc.

In summary, I don't think OpenTest4J should provide any proxy generation utilities. Third parties would of course have the option to implement proxies on their own.

As for interface default methods in Java 8+, that wouldn't get us very far: the expected and actual values are internal state. Consequently, every implementation of the interface would still have to implement getExpected() and getActual(). In other words, the only methods that are candidates for default implementations are isExpectedDefined() and isActualDefined().

@kriegfrj
Copy link
Author

kriegfrj commented May 6, 2019

Looking around, it also occurred to me that implementing something along these lines might be a potential long-term migration path to achieve #4 while maintaining backwards compatibility in the interim.

kriegfrj added a commit to kriegfrj/opentest4j that referenced this issue May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants