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

@ContextConfiguration does not merge duplicate sets of locations or classes within a test class hierarchy #32534

Closed
snicoll opened this issue Mar 26, 2024 · 4 comments
Labels
in: test Issues in the test module status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@snicoll
Copy link
Member

snicoll commented Mar 26, 2024

In our efforts to get rid of bean overriding in the core framework, I've found that test classes using @ContextConfiguration in a hierarchy with the same location means that the ApplicationContext will load and process duplicate locations several times.

This is actually the case in our own test suite, triggering a bean overriding check that I have locally for something that it shouldn't). See ClassPathResourceSpringJUnit4ClassRunnerAppCtxTests that provides the same XML location as its parent which causes the XML configuration file to be loaded twice.

@snicoll snicoll added in: test Issues in the test module type: bug A general bug labels Mar 26, 2024
@snicoll snicoll added this to the 6.2.x milestone Mar 26, 2024
@sbrannen
Copy link
Member

Code related to this can be found in:

  • AbstractTestContextBootstrapper.buildMergedContextConfiguration(Class<?>, List<ContextConfigurationAttributes>, MergedContextConfiguration, CacheAwareContextLoaderDelegate, boolean)
  • MergedContextConfiguration.processStrings(String[])

andrea-mauro added a commit to andrea-mauro/spring-framework that referenced this issue Mar 29, 2024
Prior to this commit the same location could be added twice and this would cause the Context loader to load it multiple times.

See spring-projectsgh-32534
@andrea-mauro
Copy link
Contributor

andrea-mauro commented Mar 29, 2024

Do you think following a similar approach to what's done on activeProfiles would work? You can see this proposal in the branch above

@sbrannen
Copy link
Member

sbrannen commented Mar 30, 2024

Do you think following a similar approach to what's done on activeProfiles would work?

No, I think it's unfortunately a bit more involved than that. Otherwise, we would have enforced uniqueness with a LinkedHashSet for both the locations and classes a long time ago.

Our goal here is actually not to enforce uniqueness within the entire set of locations or classes, because the duplicate presence of a configuration location or class can be intentional (or at least necessary) due to nuances of the overall configuration -- for example, because that's how the production application works. (Note: I'm not saying that any form of duplication is ideal, but I recall that we didn't go for strict uniqueness because of certain use cases in the wild.)

Rather, we want to detect when a certain class in the test class hierarchy declares the same set of locations or classes, in the exact same order, as its superclass. If we detect such accidental duplication, we then want to remove the duplication.

As I hinted at in my previous comment, we would likely need to build that logic into AbstractTestContextBootstrapper.buildMergedContextConfiguration(...).


As a side note, I suppose we would also want to remove any adjacent, duplicate configuration locations or classes.

For example, @ContextConfiguration(locations = {"config1.xml", "config1.xml", "config2.xml"}) and @ContextConfiguration(classes = {"Config1", "Config1", "Config2"}) should probably never load config1.xml and Config1 twice.

@sbrannen sbrannen added type: enhancement A general enhancement and removed type: bug A general bug labels Mar 30, 2024
@sbrannen sbrannen changed the title @ContextConfiguration does not merge identical locations in a single entry @ContextConfiguration does not merge duplicate sets of locations or classes within a test class hierarchy Mar 30, 2024
@snicoll
Copy link
Member Author

snicoll commented Apr 2, 2024

All things considered, we don't believe this to be a good change. It's more consistent to do what the users has defined in their configuration, for consistency with what's happening with production code. For hierarchical test cases, like this one, inheritLocations should be used to disable what's coming from the parent if necessary.

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Apr 2, 2024
@snicoll snicoll removed this from the 6.2.x milestone Apr 2, 2024
@snicoll snicoll added the status: declined A suggestion or change that we don't feel we should currently apply label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants