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

GH-9492: Fix @SpringIntegration in @Nested test #9506

Closed
wants to merge 2 commits into from

Conversation

mtomik
Copy link
Contributor

@mtomik mtomik commented Sep 24, 2024

Hi,
this Pull Request aims to fix issue with using @SpringIntegration test annotation in conjuction with @nested classes
more information can be found here #9492

@pivotal-cla
Copy link

@mtomik Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@mtomik Thank you for signing the Contributor License Agreement!

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this! The problem is really not so easy to spot.
We ended up with @onobc for the same conclusion as you did.

We will probably ask @sbrannen to improve docs for the @NestedTestCinfiguration to mention that this TestContextAnnotationUtils is important to use in custom annotation processors if we’d like make them working with @Nested as well.

We still would like to see some test suite to cover this functionality and supplement the fix.

You are right, something like AMQP cannot be used in this module, but that still is out of the annotation in the subject responsibility. The listener container you are mentioning in your test is definitely not an AbstractEndpoint we worry here about.

…ntext

Switches `MockIntegrationContextCustomizerFactory` to use
`TestContextAnnotationUtils` in order to properly support
`@NestedTestConfiguration` semantics.

Adds an integration test with a base class and several
`@Nested` inner test classes to verify the various
permutations of `@SpringIntegrationTest` inherit/override
behavior when used w/ `@NestedTestConfiguration`.
@onobc
Copy link
Contributor

onobc commented Sep 24, 2024

Thank you for looking into this! The problem is really not so easy to spot. We ended up with @onobc for the same conclusion as you did.

I agree w/ @artembilan in that It was not easy to spot and you did a great job @mtomik!

We still would like to see some test suite to cover this functionality and supplement the fix.

I went ahead and contributed this in commit 2. Me and @artembilan paired this afternoon and had the beginnings of the tests so I went ahead and wrapped them up and added to this code proposal. I verified the tests fail as expected w/o the fixes.

@sbrannen
Copy link
Member

We will probably ask @sbrannen to improve docs for the @NestedTestCinfiguration to mention that this TestContextAnnotationUtils is important to use in custom annotation processors if we’d like make them working with @Nested as well.

I've created the following issue to address that.

@mtomik
Copy link
Contributor Author

mtomik commented Sep 24, 2024

Thanks guys for quick feedback 👍

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Thank you all for contribution!
Not clear from now how is going to fix the rest of review concerns, but I can do that myself on merge to make all our lives easier :smile.

* to be inherited by concrete subclasses.
*
* @author Chris Bono
* @since 6.4.0
Copy link
Member

Choose a reason for hiding this comment

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

I think we can treat this as a bug, plus this is just a testing feature, so I'm going to back port the fix down to 6.2.x.
Therefore @since 6.2.10 over here.

* when used with {@link Nested} and {@link NestedTestConfiguration}.
*
* @author Chris Bono
* @since 6.4.0
Copy link
Member

Choose a reason for hiding this comment

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

DITTO

}

@Nested
@NestedTestConfiguration(NestedTestConfiguration.EnclosingConfiguration.INHERIT)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this specific test since INHERIT is there by default.

}

@Nested
@NestedTestConfiguration(NestedTestConfiguration.EnclosingConfiguration.INHERIT)
Copy link
Member

Choose a reason for hiding this comment

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

This line is redundant since INHERIT is by default.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

I'm taking this PR locally for final touches.

Thank you all for contribution!

@artembilan
Copy link
Member

Merged as 3a8e3ab.
And back-ported to 6.3.x & 6.2.x respectively.
Thank you again for the contribution!

@artembilan artembilan closed this Sep 27, 2024
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.

5 participants