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

Issue 2544 #2545

Merged
merged 4 commits into from Jan 18, 2022
Merged

Issue 2544 #2545

merged 4 commits into from Jan 18, 2022

Conversation

jspyeatt
Copy link
Contributor

Fixed issue 2544.

Mockito.mockConstructionWithAnswer() has a bug when there are more than 2 invocations possible.

If you run the ConstructionMockTest.testConstructionMocDefaultAnswerMultiple() as this:

    public void testConstructionMockDefaultAnswerMultiple() {
        try (MockedConstruction<Dummy> ignored = Mockito.mockConstructionWithAnswer(Dummy.class, 
              invocation -> "bar", 
              invocation -> "qux")) {
            assertEquals("bar", new Dummy().foo());
            assertEquals("qux", new Dummy().foo());
            assertEquals("qux", new Dummy().foo());
        }
    }

It works fine.
But if you change it to this:

    public void testConstructionMockDefaultAnswerMultiple() {
        try (MockedConstruction<Dummy> ignored = Mockito.mockConstructionWithAnswer(Dummy.class, 
               invocation -> "bar", 
               invocation -> "qux", 
               invocation -> "baz")) {
            assertEquals("bar", new Dummy().foo());
            assertEquals("qux", new Dummy().foo());
            assertEquals("baz", new Dummy().foo());
            assertEquals("baz", new Dummy().foo());
        }
    }

It will fail when trying to assert qux. It's because there is an error in the conditional logic in the else if (context.getCount() >= additionalAnswers.length). It should be > not >=.

…tructionWithAnswer().

If you build a mockConstructionWithAnswer with more than 1 additionalAnswers there was a logic error
whereby the method would return the wrong value when making the second last invocation. It would
accidentally think it's at the end an only return the last answer, never returning the second
last answer when it should.

The changes I made to ConstructionMockTest.java now exercises this condition.
…tructionWithAnswer().

If you build a mockConstructionWithAnswer with more than 1 additionalAnswers there was a logic error
whereby the method would return the wrong value when making the second last invocation. It would
accidentally think it's at the end an only return the last answer, never returning the second
last answer when it should.

The changes I made to ConstructionMockTest.java now exercises this condition.
Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and easy fix! Only a tiny nit with regards to the test.

@@ -50,10 +50,11 @@ public void testConstructionMockDefaultAnswer() {

@Test
public void testConstructionMockDefaultAnswerMultiple() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we retain the original test (e.g. 2 invocation definitions and invoking it 3 times) and instead add a new regression test which is this new format? That way we ensure that both scenarios still work, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

I restored testConstructionMockDefaultAnswer() in ConstructionMockTest
to it's original 2-invocation form.

I added new test, testConstructionMockDefaultAnswerMultipleMoreThanTwo()
to demonstrate the code is now fixed when more than 2 invocations
are in place.
Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@TimvdLippe TimvdLippe merged commit 5505941 into mockito:main Jan 18, 2022
RomaZe added a commit to arenadata/pxf that referenced this pull request Jul 25, 2023
Spring Boot Version 2.4.3 adds org.mockito package with version 3.6.8. There is a bug with this version: mockito/mockito#2545
So we need to keep additional invocation.
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

Successfully merging this pull request may close these issues.

None yet

3 participants