Skip to content
This repository was archived by the owner on Feb 23, 2023. It is now read-only.

Fix MergedContextConfiguration cache key for runtime contexts #1341

Closed
wants to merge 9 commits into from
Closed

Fix MergedContextConfiguration cache key for runtime contexts #1341

wants to merge 9 commits into from

Conversation

sbrannen
Copy link
Contributor

@sbrannen sbrannen commented Dec 5, 2021

This is a fix for #1262.

A more detailed commit message will be crafted after peer review.

@sbrannen sbrannen requested a review from snicoll December 5, 2021 14:41
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 5, 2021
@sbrannen
Copy link
Contributor Author

sbrannen commented Dec 6, 2021

Web support is not working with the status quo, so I'll have to take another look.

Caused by: java.lang.ClassCastException: class org.springframework.aot.test.AotMergedContextConfiguration cannot be cast to class org.springframework.test.context.web.WebMergedContextConfiguration (org.springframework.aot.test.AotMergedContextConfiguration and org.springframework.test.context.web.WebMergedContextConfiguration are in unnamed module of loader 'app')
	at org.springframework.aot.test.boot.AotSpringBootConfigContextLoader$WebConfigurer.configure(AotSpringBootConfigContextLoader.java:135)
	at org.springframework.aot.test.boot.AotSpringBootConfigContextLoader.loadContext(AotSpringBootConfigContextLoader.java:103)
	at org.springframework.aot.test.boot.AotSpringBootConfigContextLoader.loadContext(AotSpringBootConfigContextLoader.java:50)
	at org.springframework.aot.test.AotCacheAwareContextLoaderDelegate.loadContextInternal(AotCacheAwareContextLoaderDelegate.java:62)
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:124)
	... 68 common frames omitted

@sbrannen sbrannen marked this pull request as draft December 6, 2021 09:43
@sbrannen sbrannen force-pushed the issues/gh-1262-AOT-MergedContextConfiguration branch from fe9b0df to dd5fdb5 Compare December 6, 2021 12:12
@sbrannen sbrannen marked this pull request as ready for review December 6, 2021 12:13
@sbrannen sbrannen force-pushed the issues/gh-1262-AOT-MergedContextConfiguration branch from dd5fdb5 to 855495b Compare December 6, 2021 12:17
snicoll
snicoll reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

As requested by @sdeleuze, here's a first review. Happy to chat if more details are needed.

...tive/src/test/java/org/springframework/aot/test/AotCacheAwareContextLoaderDelegateTests.java
MergedContextConfiguration mergedConfig = createMergedContextConfiguration(SampleTest.class);
assertThat(delegate.loadContext(mergedConfig)).isSameAs(this.applicationContext);

// loadContext() must actually be invoked with the original MergedContextConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

It will take me a bit of time to process this but, at a quick glance, I generally feel that a "lot' of explanations like that in a test is the sign that something else should be made more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough.

I'll move the comments to actual Javadoc in the production code.

@sdeleuze
Copy link
Contributor

sdeleuze commented Dec 7, 2021

Please let me know when this PR is ready for final review and merge.

@sdeleuze sdeleuze added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 7, 2021
@sbrannen
Copy link
Contributor Author

sbrannen commented Dec 7, 2021 •
edited
Loading

Please let me know when this PR is ready for final review and merge.

I'll post here when it's ready.

@snicoll snicoll added this to the 0.11.0 milestone Dec 7, 2021
@snicoll snicoll mentioned this pull request Dec 7, 2021
Closed
@snicoll snicoll changed the title Introduce AotMergedContextConfiguration for tests in AOT mode Fix MergedContextConfiguration cache key for runtime contexts Dec 7, 2021
@sbrannen
Introduce AotMergedContextConfiguration for tests in AOT mode
320df02 
See spring-atticgh-1262
@sbrannen
Reinstate @disabled integration test for context caching in AOT test …
12a2d36 
…mode

See spring-atticgh-1262
@sbrannen
Revise TestContextAotProcessor according to review feedback
3f4526d 
See spring-atticgh-1262
@sbrannen
Revise TestContextAotProcessorTests according to review feedback
0f06bed 
See spring-atticgh-1262
@sbrannen
Revise AotCacheAwareContextLoaderDelegate according to review feedback
1830ad8 
See spring-atticgh-1262
@sbrannen
Reduce duplication in AotContextLoader according to review feedback
3ef445f 
See spring-atticgh-1262
@sbrannen
Rename AotContextLoader to AotTestMappings and update Javadoc
d9d03db 
See spring-atticgh-1262
@sbrannen
Introduce unit tests for AotMergedContextConfiguration
905fbba 
See spring-atticgh-1262
@sbrannen
Introduce integration tests for AotCacheAwareContextLoaderDelegate
56f3be2 
These tests also indirectly verify the implementation of equals() and
hashCode() for instances of AotMergedContextConfiguration created by
AotCacheAwareContextLoaderDelegate.

See spring-atticgh-1262
@sbrannen sbrannen force-pushed the issues/gh-1262-AOT-MergedContextConfiguration branch from 855495b to 56f3be2 Compare December 7, 2021 15:50
@sbrannen
Copy link
Contributor Author

sbrannen commented Dec 7, 2021

This PR is ready for a final review.

The only remaining task is an update to the Javadoc (see #1341 (comment)), but that can be done at later date if necessary.

snicoll pushed a commit that referenced this pull request Dec 7, 2021
@sbrannen @snicoll
Introduce AotMergedContextConfiguration for tests in AOT mode
36dff57 
See gh-1341
snicoll added a commit that referenced this pull request Dec 7, 2021
@snicoll
Polish "Introduce AotMergedContextConfiguration for tests in AOT mode"
4280ad5 
See gh-1341
@snicoll snicoll closed this in 07a4e66 Dec 7, 2021
@snicoll
Copy link
Contributor

snicoll commented Dec 7, 2021

Thanks Sam, this looks good.

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

@snicoll snicoll snicoll left review comments

Assignees
No one assigned
Labels
type: bug A general bug
Milestone
0.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sbrannen @sdeleuze @snicoll @spring-projects-issues