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

Fixes #2281: Make MockedConstruction stubs close on demand #2442

Merged
merged 5 commits into from Oct 6, 2021

Conversation

temp-droid
Copy link
Contributor

@temp-droid temp-droid commented Oct 5, 2021

MockedConstruction should behave like MockedStatic when we use @mock on it.

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

Allows this kind of test to work in multiple test files:

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.MockedConstruction;
import org.mockito.MockedStatic;
import org.mockito.junit.jupiter.MockitoExtension;

import static org.junit.jupiter.api.Assertions.assertNotNull;

@ExtendWith(MockitoExtension.class)
public class Test1 {

    @Test
    void testStatic(@Mock MockedStatic<ClassToMock> mockedStatic) {
        assertNotNull(mockedStatic);
    }

    @Test
    void testMockedConstruction(@Mock MockedConstruction<ClassToMock> mockConstruction) {
//        mockConstruction.closeOnDemand();
        assertNotNull(mockConstruction);
    }
}

Otherwise the mockConstruction object won't be closed and generate error described in #2281.

MockedConstruction should behave like MockedStatic when we use @mock on it.
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.

Please add the regression test that you mentioned in your PR description to our test suite in https://github.com/mockito/mockito/tree/main/subprojects/junit-jupiter/src/test/java/org/mockitousage

@TimvdLippe TimvdLippe requested a review from raphw October 5, 2021 20:45
@temp-droid
Copy link
Contributor Author

Hi @TimvdLippe , tests have been added. Feel free to mention if something needs to be changed!

Sorry if I messed up with the merge (not sure why it was needed), I hope we can squash everything before merging to hide it?

Copy link
Member

@raphw raphw left a comment

Choose a reason for hiding this comment

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

Obvious oversight. Good to merge.

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.

Change looks good to me. We can clean up the commit message when we squash-merge, so don't worry about that 😄

Only a small nit with regard to the missing license. Other than that this is ready to go.

* Tests to assert that ScopedMock mocks are properly closed on scope exit.
*/
@ExtendWith(MockitoExtension.class)
class CloseOnDemandTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Argh, my apologies. Since this only works for the inline mockmaker, the test file needs to be in https://github.com/mockito/mockito/tree/main/subprojects/junitJupiterInlineMockMakerExtensionTest/src/test/java/org/mockitousage

Please run ./gradlew build locally to ensure that the tests pass before you update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, and thank you for pointing that out, I was getting confused by the error message.

Also the project was building fine locally, which I find suspicious based on the error message I receive on Github.

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 for the fix!

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

4 participants