[🚀 Feature]: [java] Allow EventFiringDecorator to throw exceptions fired by WebDriverListeners

[giulong]

Description

As stated in EventFiringDecorator javadocs:

Listeners can't affect driver behavior too much. They can't throw any exceptions (they can, but the decorator suppresses these exceptions)

There could be scenarios where a listener should actually be able to break the execution, and I think the decision of suppressing or allowing exceptions to be thrown should be left to the user, rather than imposed internally.

Context

I'm implementing Visual Regression Testing in giulong/spectrum#447, for which:

1. the user configures events to listen to, let's say afterClick.
2. the first execution of each test takes a screenshot after each click. These act as snapshots.
3. the subsequent executions of the same tests will take screenshots in the same moments, and compare them to the snapshots. If they don't match, an exception is thrown.

To achieve this, a decorated WebDriver listens to the events configured by the user (afterClick). The associated WebDriverListener intercepts those events, and uses the original undecorated driver instance to take a screenshot. Using the undecorated driver is needed because taking the screenshot is an operation performed under the hood by the framework, and must not interfere with user interactions. So, no WebDriverListener is actually used to intercept these screenshots being taken. To summarize:

1. a decorated WebDriver is used to run tests.
2. a WebDriverListener intercepts afterClick.
3. the undecorated driver is used to take a screenshot.
4. If the screenshot doesn't match with the corresponding snapshot, an exception is thrown.
5. 💥 issue: the user just gets a warning logged, instead of having the test failing fast.

Proposal

Let users decide whether to ignore exceptions or not, by adding a method to WebDriverListener to configure the behavior of each listener:

default boolean throwsExceptions() {
  return false;
}

Considerations:

• having this at listener level instead of directly into EventFiringDecorator allows for a more fine grained configuration: we can have many listeners attached to the same decorator, with only a subset actually rethrowing exceptions.

• returning false by default means no breaking change is introduced. Whenever an exception is thrown in a listener execution, only if that listener is configured to throw exceptions, the EventFiringDecorator will rethrow the original one:

  private void callListenerMethod(Method m, WebDriverListener listener, Object[] args) {
    try {
      m.invoke(listener, args);
    } catch (Throwable t) {
      LOG.log(Level.WARNING, t.getMessage(), t);
  
      // This is the new part
      if (listener.throwsExceptions()) {
        throw new WebDriverListenerException(m.getName(), t);
      }
    }
  }

• WebDriverListenerException is a RuntimeException to avoid introducing breaking changes.

Have you considered any alternatives or workarounds?

1. I tried extending EventFiringDecorator but its current design, with basically just private methods such as fireBeforeEvents and fireAfterEvents that do all the work including suppressing exceptions, does not allow me to effectively hook onto the listeners execution logic.

2. I also tried to override the onError method in my own decorator, but I have no access to listeners, which is a private field with no getter. Moreover, onError is not triggered in such cases. So, with regard to the PR attached, I tried another approach as well, that is programmatically triggering onError like this:

     private void callListenerMethod(Method m, WebDriverListener listener, Object[] args)
         throws Throwable {
       try {
         m.invoke(listener, args);
       } catch (Throwable t) {
         LOG.log(Level.WARNING, t.getMessage(), t);
     
         // These lines added
         if (t instanceof InvocationTargetException) {
           this.onError(getDecoratedDriver(), m, args, (InvocationTargetException) t);
         }
       }
     }

   this triggers onError method on each listener in turn:

   @Override
   public Object onError(
       Decorated<?> target, Method method, Object[] args, InvocationTargetException 
       throws Throwable {
     listeners.forEach(
         listener -> {
           try {
             listener.onError(target.getOriginal(), method, args, e);
           } catch (Throwable t) {
             LOG.log(Level.WARNING, t.getMessage(), t);
           }
         });
     return super.onError(target, method, args, e);
   }

   This works, but it's a breaking change since we're always throwing exceptions. Not to mention that I don't think this is the way onError is intended to be used.

The only workaround I see would be to copy the entire code of EventFiringDecorator in an own-written decorator that just doesn't swallow exceptions. But I'd like to avoid that and keep using the internal one, since it's already doing all the work.

Of course, in case I'm missing something and there's any other smart way of achieving this, just tell me.

[selenium-ci]

@giulong, thank you for creating this issue. We will troubleshoot it as soon as we can.

Selenium Triage Team: remember to follow the Triage Guide