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.

Sign up for GitHub

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

Jump to bottom

Inject mock annotations cleanup #33964

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Inject mock annotations cleanup #33964

merged 1 commit into from
Jun 15, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jun 12, 2023

  • introduce io.quarkus.test.InjectMock
  • deprecate io.quakus.test.junit.mockito.InjectMock
  • remove io.quarkus.test.component.ConfigureMock

mkouba
mkouba commented Jun 12, 2023
View reviewed changes
...ramework/junit5/src/main/java/io/quarkus/test/junit/internal/VerifyMockitoMocksCallback.java
}
current = current.getSuperclass();
if (!injectMockFields.isEmpty()) {
throw new IllegalStateException(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this because the @io.quarkus.test.InjectMock annotation is on the class path even if not supported (i.e. no quarkus-junit5-mockito dependency)...

@geoand
Copy link
Contributor

geoand commented Jun 12, 2023

I have not been following the latest discussions because I was out for a few days, so I'll let others review before I do

@glefloch
Copy link
Member

glefloch commented Jun 12, 2023 •
edited
Loading

This looks cool to have a common annotation for mocking. I'm maintaining this project which make it possible to use Mockk as mocking library (usually used in kotlin).
With this, we could rely on the same InjectMock annotation.
There is also an @InjectSpy annotation, will it be part of the common package too ?

@mkouba
Copy link
Contributor Author

mkouba commented Jun 12, 2023

There is also an @InjectSpy annotation, will it be part of the common package too ?

That is a very good question. I don't know.

For example, QuarkusComponentTest does not support mockito spies.

And given the fact that Mockito#spy() javadoc states: "Real spies should be used carefully and occasionally, for example when dealing with legacy code."... 🤷

@github-actions
Copy link

github-actions bot commented Jun 12, 2023 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@holly-cummins
Copy link
Contributor

I think this looks great.

Adding the back-link to #33827. Once this merges I'll tidy up my table there.

@holly-cummins holly-cummins mentioned this pull request Jun 13, 2023
Closed
@mkouba mkouba force-pushed the new-inject-mock branch from 42fb408 to 0ae9eeb Compare June 13, 2023 14:24
@mkouba mkouba mentioned this pull request Jun 13, 2023
Open
@mkouba
Copy link
Contributor Author

mkouba commented Jun 13, 2023

FTR I've created #34018 for a common @InjectSpy.

@mkouba mkouba marked this pull request as ready for review June 13, 2023 14:28
@mkouba mkouba added this to the 3.2 - main milestone Jun 13, 2023
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba mkouba force-pushed the new-inject-mock branch from 0ae9eeb to 787aff0 Compare June 13, 2023 15:23
@mkouba mkouba requested a review from Ladicek June 13, 2023 15:23
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 13, 2023
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba mkouba force-pushed the new-inject-mock branch from 787aff0 to bbb6adb Compare June 14, 2023 06:45
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Inject mock annotations cleanup
610a4a5 
- introduce io.quarkus.test.InjectMock
- deprecate io.quakus.test.junit.mockito.InjectMock
- remove io.quarkus.test.component.ConfigureMock
@mkouba mkouba force-pushed the new-inject-mock branch from bbb6adb to 610a4a5 Compare June 15, 2023 08:30
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 15, 2023

Failing Jobs - Building 610a4a5

Status Name Step Failures Logs Raw logs
✖ Native Tests - Windows - RESTEasy Jackson Setup GraalVM ⚠️ Check → Logs Raw logs

@mkouba mkouba merged commit 92a0fa4 into quarkusio:main Jun 15, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 15, 2023
luneo7
luneo7 reviewed Jul 24, 2023
View reviewed changes
...est/junit/mockito/internal/SingletonToApplicationScopedTestBuildChainCustomizerProducer.java
for (AnnotationInstance instance : instances) {
if (instance.target().kind() != AnnotationTarget.Kind.FIELD) {
continue;
}
if (instance.name().equals(MOCKITO_CONFIG)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there was a typo here maybe this should have been:

if (instance.name().equals(MOCKITO_CONFIG) && instance.target().asField().hasAnnotation(DEPRECATED_INJECT_MOCK)) {
   continue;
}

As I think that we shouldn't process @MockConfig and the deprecated @InjectMock twice as it sets the same configs, but we should process the @MockConfig if being used in conjunction with the new @InjectMock to tweak the mock configuration.
Since now using @MockitoConfig to convert scopes doesn't work =/
Also saw that there is no test for converting scopes, so this it wasn't caught before.
Good thing is that there is still the deprecated @InjectMock as we can use it do to scope conversion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luneo7 luneo7 luneo7 left review comments

@holly-cummins holly-cummins holly-cummins approved these changes

@Ladicek Ladicek Ladicek approved these changes

@geoand geoand Awaiting requested review from geoand

Assignees
No one assigned
Labels
area/documentation area/testing
Projects
None yet
Milestone
3.2.0.CR1
Development

Successfully merging this pull request may close these issues.
6 participants
@mkouba @geoand @glefloch @holly-cummins @Ladicek @luneo7