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

'MismatchReportingTestListener' has already been added and not removed using MockitoJUnitRunner #1767

Closed
torakiki opened this issue Aug 27, 2019 · 21 comments

Comments

@torakiki
Copy link

@torakiki torakiki commented Aug 27, 2019

  • The mockito message in the stacktrace have useful information, but it didn't help
  • The problematic code (if that's possible) is copied here;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasProperty;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.hamcrest.MockitoHamcrest.argThat;

import java.io.File;
import java.io.IOException;
import java.util.function.Consumer;

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.pdfsam.support.params.SingleOutputTaskParametersBuilder;
import org.pdfsam.test.InitializeAndApplyJavaFxThreadRule;

@RunWith(MockitoJUnitRunner.class)
public class BrowsablePdfOutputFieldTest {
    @Rule
    public TemporaryFolder folder = new TemporaryFolder();
    @Rule
    public InitializeAndApplyJavaFxThreadRule fxThread = new InitializeAndApplyJavaFxThreadRule();
    @Mock
    private SingleOutputTaskParametersBuilder<?> builder;
    @Mock
    private Consumer<String> onError;

    @Test
    public void valid() throws IOException {
        BrowsablePdfOutputField victim = new BrowsablePdfOutputField();
        File value = folder.newFile("test.pdf");
        victim.getTextField().setText(value.getAbsolutePath());
        victim.apply(builder, onError);
        verify(builder).output(argThat(hasProperty("destination", equalTo(value))));
        verify(onError, never()).accept(anyString());
    }

    @Test
    public void invalid() {
        BrowsablePdfOutputField victim = new BrowsablePdfOutputField();
        victim.enforceValidation(true, true);
        victim.getTextField().setText("ChuckNorris");
        victim.apply(builder, onError);
        verify(builder, never()).output(any());
        verify(onError).accept(anyString());
    }
}

I migrated from 1.10.19 to the 3.0.0 version and I have few tests using the MockitoJUnitRunner which are failing with the following stacktrace:

org.mockito.exceptions.misusing.RedundantListenerException: 
Problems adding Mockito listener.
Listener of type 'MismatchReportingTestListener' has already been added and not removed.
It indicates that previous listener was not removed according to the API.
When you add a listener, don't forget to remove the listener afterwards:
  Mockito.framework().removeListener(myListener);
For more information, see the javadoc for RedundantListenerException class.
	at org.mockito.internal.runners.DefaultInternalRunner$1$1.evaluate(DefaultInternalRunner.java:41)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
	at org.pdfsam.test.JavaFXThreadRule$1.lambda$evaluate$0(JavaFXThreadRule.java:48)
	at com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:389)
	at com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
	at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)
	at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
	at com.sun.glass.ui.gtk.GtkApplication.lambda$runLoop$11(GtkApplication.java:277)
	at java.base/java.lang.Thread.run(Thread.java:835)

It seems something internal, I didn't add or remove any MismatchReportingTestListener and moreover the same code was working with the 1.10.19 and is working if I remove the MockitoJUnitRunner and replace the @Mock annotation with a Mockito.mock in a @Before method.

@TimvdLippe

This comment has been minimized.

Copy link
Contributor

@TimvdLippe TimvdLippe commented Aug 31, 2019

A lot of updates exist between 1.10.19 and 3.0.0. Could you bisect the versions and figure out which minor version introduced the issue?

@torakiki

This comment has been minimized.

Copy link
Author

@torakiki torakiki commented Sep 2, 2019

2.26.0 looks like the one introducing this issue, all green in 2.25.1

@mockitoguy

This comment has been minimized.

Copy link
Member

@mockitoguy mockitoguy commented Sep 26, 2019

@torakiki, can you provide us a simpler test that reproduces the issue? We are looking for a test without dependencies such as InitializeAndApplyJavaFxThreadRule, so that we can run & reproduce it in Mockito project.

Thanks for reporting!

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Oct 15, 2019

@torakiki any luck? I'm having the same issue, upgrading from 2.23.4

@mockitoguy here's a simple test reproducing the issue in my case:

@RunWith(MockitoJUnitRunner.class)
public class UuidHelperTest {


    @Test
    public void should_Get_16_Bytes_From_a_UUID() {
        UUID uuid = UUID.randomUUID();

        byte[] result = UuidHelper.getBytesFromUUID(uuid);

        assertThat(result.length, is(16));
    }

}

public class UuidHelper {

    public static final int UUID_SIZE_IN_BYTES = 16;


    public static byte[] getBytesFromUUID(UUID uuid) {
        ByteBuffer bb = ByteBuffer.wrap(new byte[UUID_SIZE_IN_BYTES]);
        bb.putLong(uuid.getMostSignificantBits());
        bb.putLong(uuid.getLeastSignificantBits());

        return bb.array();
    }

}
@mockitoguy

This comment has been minimized.

Copy link
Member

@mockitoguy mockitoguy commented Oct 17, 2019

@SharonHart, this test works fine with current Mockito (3.0). Can you provide a test that fails with current Mockito? Thank you!

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Oct 17, 2019

@mockitoguy It works with the current Mockito version for me as well, on a clean project.
But in one specific project it doesn't, after a similar upgrade, as the one mentioned in this issue.

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 5, 2019

@marcphilipp, @TimvdLippe Maybe this PR is related?
https://github.com/mockito/mockito/pull/1672/files#diff-f266327a2222bf41843bb64e18ba766eL59
It was merged between the versions @torakiki mentioned here.
Used to removeListener in case of test failure during setup, the same thing as the exception states that needs to be done.

Edit: I've reverted the commit locally and published to m2. Everything works now. Can this be reverted or checked for root cause?

@TimvdLippe

This comment has been minimized.

Copy link
Contributor

@TimvdLippe TimvdLippe commented Nov 5, 2019

@SharonHart Which version of JUnit are you using? We had a regression since 4.13 which is why we made the changes. See junit-team/junit4#1599

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 6, 2019

@TimvdLippe JUnit 4.12

By the way, seems like what's causing the regression for me is the withBefores statement part, not the 'started' field removal in the RunListener. But, the regression you've mention was with the prior, right?

@marcphilipp

This comment has been minimized.

Copy link
Contributor

@marcphilipp marcphilipp commented Nov 6, 2019

@SharonHart Could you please provide a sample project so we can reproduce the issue?

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 6, 2019

@marcphilipp Unfortunately, I can't. I tried to copy the test to a clean project and it didn't reproduce.
Happening only in command line build, in one project which I can't share due to size and internal dependencies.
I can verify if a fix works though, the same way I've managed to check if reverting the commit fixed the issue.

@marcphilipp

This comment has been minimized.

Copy link
Contributor

@marcphilipp marcphilipp commented Nov 6, 2019

Are you running tests in parallel or anything like that?

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 6, 2019

Yes, in our CI pipeline. But reproduced locally with a single executor. It might be that the single one is running modules in parallel. I'm not sure.

@marcphilipp

This comment has been minimized.

Copy link
Contributor

@marcphilipp marcphilipp commented Nov 6, 2019

How are you running those tests? Maven Surefire?

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 6, 2019

No, with gradle, without extra plugins

@marcphilipp

This comment has been minimized.

Copy link
Contributor

@marcphilipp marcphilipp commented Nov 6, 2019

What happens when you upgrade to JUnit 4.13-rc-1?

@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 6, 2019

Same.

@hotzst

This comment has been minimized.

Copy link

@hotzst hotzst commented Nov 10, 2019

While I cannot provide a sample project to reproduce this issue, here are some observations that I made when running into the same issue:

  1. I seems to be related one of the @Rule: I use a similar rule to what @torakiki has in his example:
public class JavaFXThreadingRule implements TestRule {

        /**
        * Flag for setting up the JavaFX, we only need to do this once for all tests.
        */
        private static boolean jfxIsSetup;

        @Override
        public Statement apply(Statement statement, Description description) {

            return new OnJFXThreadStatement(statement);
        }

        private static class OnJFXThreadStatement extends Statement {

            private final Statement statement;

            public OnJFXThreadStatement(Statement aStatement) {
                statement = aStatement;
            }

            private Throwable rethrownException = null;

            @Override
            public void evaluate() throws Throwable {

                if(!jfxIsSetup) {
                    setupJavaFX();

                    jfxIsSetup = true;
                }

                final CountDownLatch countDownLatch = new CountDownLatch(1);

                Platform.runLater(() -> {
                    try {
                        statement.evaluate();
                    } catch (Throwable e) {
                        rethrownException = e;
                    }
                    countDownLatch.countDown();
                });

                countDownLatch.await();

                // if an exception was thrown by the statement during evaluation,
                // then re-throw it to fail the test
                if(rethrownException != null) {
                    throw rethrownException;
                }
            }

            protected void setupJavaFX() throws InterruptedException {

                long timeMillis = System.currentTimeMillis();

                final CountDownLatch latch = new CountDownLatch(1);

                SwingUtilities.invokeLater(() -> {
                    // initializes JavaFX environment
                    new JFXPanel();

                    latch.countDown();
                });

                System.out.println("javafx initialising...");
                latch.await();
                System.out.println("javafx is initialised in " + (System.currentTimeMillis() - timeMillis) + "ms");
            }

        }
}
  1. My test class contains multiple test methods. Running them separately woks, when running them in combination (from within the IDE) the first one succeeds and then the remaining fail with this exception.
  2. Removing the rule makes all tests succeed safe the ones that actually require the rule.
@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 10, 2019

@hotzst Nice! I have managed to find that for me as well, a MethodRule we're implementing is what caused this. Question is, how should it be changed for the updated Mockito version.

import org.joda.time.DateTimeZone;
import org.junit.rules.MethodRule;
import org.junit.runners.model.FrameworkMethod;
import org.junit.runners.model.Statement;
import org.mockito.Mockito;
import org.mockito.internal.junit.MockitoTestListener;

import java.util.TimeZone;

/*
* When adding this rule to a Junit test, any @Test method annotated with @TimeZoned will be ran multiple times, each time
* with a different system timezone.
* This rule is useful to ensure you are handling all timezones correctly.
* Note: for Mockito, you may need to use Mockito.reset() if you are using verify().
* */
public class TimeZoneUpdateRule implements MethodRule {

    final private TimeZone timeZone;
    final private DateTimeZone dateTimeZone;
    private final static String[] allTimeZones = {"UTC", "Asia/Jerusalem", "Europe/London", "US/Pacific", "EST5EDT", "US/Eastern",
            "Australia/Sydney", "US/Central", "Asia/Tokyo", "America/Chicago", "GMT",
            "Europe/Amsterdam", "Europe/Berlin", "Brazil/East", "Asia/Jakarta", "Europe/Athens",
            "Australia/Queensland", "Asia/Singapore", "Asia/Bangkok", "America/Bogota",
            "America/Los_Angeles", "Asia/Hong_Kong", "America/New_York",
            "America/Argentina/Buenos_Aires"};

    public TimeZoneUpdateRule() {
        timeZone = TimeZone.getDefault();
        dateTimeZone = DateTimeZone.getDefault();
    }

    public void reset() {
        setTimeZone(timeZone, dateTimeZone);
    }

    protected void after() {
        reset();
    }

    private void setTimeZone(String timeZoneID) {
        setTimeZone(TimeZone.getTimeZone(timeZoneID), DateTimeZone.forID(timeZoneID));
    }

    private void setTimeZone(TimeZone timeZone, DateTimeZone dateTimeZone) {
        System.setProperty("user.timezone", timeZone.getID());
        TimeZone.setDefault(timeZone);
        DateTimeZone.setDefault(dateTimeZone);
    }

    private static String[] getTimeZones() {
        return allTimeZones;
    }


    @Override
    public Statement apply(final Statement st, final FrameworkMethod method, Object target) {
        return new Statement() {
            public void evaluate() throws Throwable {
                TimeZoned timeZoned = method.getAnnotation(TimeZoned.class);
                if(timeZoned !=null){
                    String[] timeZones =  timeZoned.timeZones().length > 0  ? timeZoned.timeZones() : getTimeZones();
                    applyAllTimeZones(st, timeZones);
                }else{
                    st.evaluate();
                }
            }

        };
    }

    private void applyAllTimeZones(Statement st, String[] timezones) throws Throwable {
        try {
            for (String tz : timezones) {
                setTimeZone(tz);
                st.evaluate();
            }
        }catch (AssertionError t){
            throw new AssertionError(getMessage(), t);
        }catch (Exception e){
            throw new RuntimeException(getMessage(), e);
        }
        finally {
            after();
        }
    }

    private String getMessage() {
        return "test failed for timezone [" + TimeZone.getDefault().getID()+ "]";
    }
}
marcphilipp added a commit to marcphilipp/mockito that referenced this issue Nov 10, 2019
Some rules evaluate the base statement multiple times, e.g. to execute
tests repeatedly. The changes made in mockito#1672 led to an exception in such
cases because the `MockitoListener` was registered multiple times. Now,
we only add the listener the first time the statement is evaluated in
order to restore the old behavior.

Fixes mockito#1767.
@marcphilipp

This comment has been minimized.

Copy link
Contributor

@marcphilipp marcphilipp commented Nov 10, 2019

Thanks for the additional info. I think #1821 should restore the old behavior.

TimvdLippe added a commit that referenced this issue Nov 10, 2019
* Guard against multiple evaluations of before statement

Some rules evaluate the base statement multiple times, e.g. to execute
tests repeatedly. The changes made in #1672 led to an exception in such
cases because the `MockitoListener` was registered multiple times. Now,
we only add the listener the first time the statement is evaluated in
order to restore the old behavior.

Fixes #1767.

* Reset listener when removing it
@SharonHart

This comment has been minimized.

Copy link

@SharonHart SharonHart commented Nov 10, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.