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

Throw error from reporter directly for single reporter #157

Open
muehmar opened this issue Aug 7, 2023 · 4 comments
Open

Throw error from reporter directly for single reporter #157

muehmar opened this issue Aug 7, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@muehmar
Copy link

muehmar commented Aug 7, 2023

Is it possible to throw the error from a reporter directly in case there is only one compatible reporter instead of catching it and wrap it into the custom SnapshotMatchException?

The reason behind this question: IDE's (in my case IntelliJ IDEA) may have the ability to provide a diff for failing tests directly (without the need to find the *snap.debug file manually and compare it with the current file) but only if the error is well known (I guess). So currently, I can add my own reporter which uses assertions from junit5 for which IntelliJ could provide the diff but as the error is wrapped in the SnapshotMatchException, Intellij wont provide any diff.

@jackmatt2
Copy link
Member

In theory a check for a single error could happen here:

https://github.com/origin-energy/java-snapshot-testing/blob/master/java-snapshot-testing-core/src/main/java/au/com/origin/snapshots/SnapshotContext.java#L91-L101

I'm not sure we'd want to do it though and it might be able to be done another way.

Have a look at this thread: #44 (comment)

Specifically using https://github.com/skyscreamer/JSONassert

@muehmar
Copy link
Author

muehmar commented Aug 8, 2023

Instead of throwing the error directly in case of single reporters, one could also unwrap the SnapshotMatchException in a test. For JUnit5 one can implement the InvocationInterceptor, where one can catch the SnapshotMatchException and unwrap it in case of a single failure. As your SnapshotExtension class is not final, I was able to extend this class and implement the method to unwrap it (and use DiffSnapshotExtension instead of SnapshotExtension):

public class DiffSnapshotExtension extends SnapshotExtension
    implements InvocationInterceptor {

  @Override
  public void interceptTestMethod(
      final Invocation<Void> invocation,
      final ReflectiveInvocationContext<Method> invocationContext,
      final ExtensionContext extensionContext)
      throws Throwable {
    try {
      invocation.proceed();
    } catch (final SnapshotMatchException e) {
      final List<Throwable> failures = e.getFailures();
      if (failures.size() == 1) {
        throw failures.get(0);
      }
      throw e;
    }
  }
}

I suspect, the SnapshotExtension class is not really designed to be extended, the alternative would be to not inherit from SnapshotExtension but one has to annotate each test class with two extensions.

Is there anything which you can think of to add support to achieve this? Probably one could provide an opt-in mechanism to map the SnapshotMatchException to a custom one? If no mapping is defined, the behaviour is the same as now.

@muehmar
Copy link
Author

muehmar commented Aug 31, 2023

Any thoughts about this? The mentioned interceptor works, one has to override also another method to intercept also parameterized tests. I implemented this in a separate Extension, so I need to add for every snapshot test class two extensions which works but is not as convenient as before (and all existing snapshot test classes need to get modified).

So I really would appreciate any support in the library concerning this. Adding the possibility to specify a custom Exception Mapper (the way it is done also for Reporters etc) does not seem to be a big deal or is it? The custom Exception mapper is just used to map the created SnapshotMatchException before it is thrown. If no Exception Mapper is configured (or a standard implementation which simply does not do any mapping) will just throw the SnapshotMatchException as is.

If you are pretty busy I could also implement the changes and open a PR if you think this is something you want to support?

@jackmatt2 jackmatt2 added the enhancement New feature or request label Jun 1, 2024
@jackmatt2
Copy link
Member

I would be happy for a PR that achieves this. Just needs to remain backward compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants