Support @TestPropertySource as a repeatable annotation

[antkorwin]

Fixed #21986 and https://jira.spring.io/browse/SPR-17454?redirect=false

I often make composite meta-annotations in my libraries and sometimes I need to define some properties in tests by the using of meta-annotation, it's difficult without the full support of inheritance and repeatable of the TestPropertySource annotation.

I tried to implement it in this PR, hope this is the right way. If not, please suggest me how to make it better.

[sbrannen]

I think the PR looks pretty good. I've requested a few minor changes.

Please update the copyright headers where appropriate as well.

[antkorwin]

@sbrannen thanks for your feedback!

I made the requested changes,
it looks like everything is ready.

[sbrannen]

I made the requested changes,

Thanks!

Unless I overlooked something, it looks like we need some tests that demonstrate/verify the override behavior when multiple @TestPropertySource annotations are present. For example, which declaration of a property named foo wins if foo is declared locally, on a superclass, and/or via a meta-annotation?

Could you please introduce such tests?

[antkorwin]

Unless I overlooked something, it looks like we need some tests that demonstrate/verify the override behavior when multiple @TestPropertySource annotations are present. For example, which declaration of a property named foo wins if foo is declared locally, on a superclass, and/or via a meta-annotation?

Could you please introduce such tests?

of course,

I added a few test cases to demonstrate how to work overriding of @TestPropertySource annotation.

[sbrannen]

I added a few test cases to demonstrate how to work overriding of @TestPropertySource annotation.

Thanks!

I'll try to review them and finish up this PR in the coming days.

[sbrannen]

@antkorwin, could it be that you copied the Javadoc for @TestPropertySources from org.junit.jupiter.api.extension.Extensions?

It sounds very familiar. 😉

[antkorwin]

could it be that you copied the Javadoc for @TestPropertySources from org.junit.jupiter.api.extension.Extensions?

I think this is due to the fact that I often have one opened window with source files from the Junit5, so to speak, for inspiration 😃

need to say that this is a real example of a well-designed and well-documented system.

[sbrannen]

could it be that you copied the Javadoc for @TestPropertySources from org.junit.jupiter.api.extension.Extensions?

I think this is due to the fact that I often have one opened window with source files from the Junit5, so to speak, for inspiration 😃

Understood. I wrote that Javadoc for JUnit 5. That's why it sounded so familiar to me.

Normally we shouldn't copy documentation verbatim from other projects, but in this case I think it's OK.

need to say that this is a real example of a well-designed and well-documented system.

😊

[sbrannen]

Current work on this PR can be viewed in the following feature branch.

https://github.com/sbrannen/spring-framework/commits/issues/gh-23320-repeatable-TestPropertySource

I had to completely rewrite TestPropertySourceUtils.resolveTestPropertySourceAttributes() in order to provide proper support for property overrides within local, directly present @TestPropertySource declarations, and that turned out to be much more involved than originally anticipated.

Another open issue is the handling of the inheritLocations and inheritProperties flags amongst repeated @TestPropertySource declarations. Before supporting @TestPropertySource as a repeatable annotation, the semantics of those flags were straightforward. Now there is some ambiguity and potential for conflict. Thus, we will have to come up with a plan for how to handle those. One idea I have in mind is to throw an exception if @TestPropertySource declarations that are directly present or meta-present on a given class (i.e., have the same aggregate index in MergedAnnotation terminology) have different values for the inheritLocations and inheritProperties flags, since that would not make sense.

Feedback is welcome.

[sbrannen]

One idea I have in mind is to throw an exception if @TestPropertySource declarations that are directly present or meta-present on a given class (i.e., have the same aggregate index in MergedAnnotation terminology) have different values for the inheritLocations and inheritProperties flags, since that would not make sense.

I in fact went that route in the latest commit on the aforementioned feature branch.

locations and properties from @TestPropertySource declarations that are directly present or meta-present on a given class are now merged into a single instance of TestPropertySourceAttributes internally, with assertions in place to ensure that such @TestPropertySource declarations do not configure different values for the inheritLocations and inheritProperties flags.

I think the semantics, and therefore the implementation, are pretty solid now, but feedback is still welcome before I document the changes and push to master.

@philwebb, I'd also appreciate feedback from you regarding use of the MergedAnnotations API in this PR (and the follow-up commits on the feature branch).

[sbrannen]

This has been merged into master in 2e476ca and overhauled in 136af0b.

[antkorwin]

I had to completely rewrite TestPropertySourceUtils.resolveTestPropertySourceAttributes() in order to provide proper support for property overrides within local, directly present @TestPropertySource declarations, and that turned out to be much more involved than originally anticipated.

I read the documentation about MergedAnnotations.stream() and I found that the result stream is ordered by the annotation distance. But your solution with aggregateIndex is looking more stable for me.

One idea I have in mind is to throw an exception if @TestPropertySource declarations that are directly present or meta-present on a given class

if we check this on the same level of @TestPropertySource present this is pretty expected that throws an exception if in different annotation present different values of these flags (on the same level). But if @TestPropertySource declared with different values on different levels (for example one in meta-annotation or parent class and another directly on class) than direct definition must override the value from meta-annotation or parent-class.

[sbrannen]

@antkorwin, Thanks again for the PR and for reviewing the subsequent changes!

I read the documentation about MergedAnnotations.stream() and I found that the result stream is ordered by the annotation distance. But your solution with aggregateIndex is looking more stable for me.

Yeah, the ordering within the MergedAnnotations.stream() is what you want most of the time, but we have a special use case here. The only way to properly support the inheritLocations and inheritProperties flags is to treat all annotations with the same aggregate index as a single, merged annotation for that aggregate index (i.e., class hierarchy level).

That is actually in line with the existing documentation for @TestPropertySource. For example, here's the Javadoc for inheritLocations:

"Whether or not test property source locations from superclasses should be inherited."

if we check this on the same level of @TestPropertySource present this is pretty expected that throws an exception if in different annotation present different values of these flags (on the same level).

Exactly. That's what the code does now: it throws an exception if there are conflicting values for those flags for all annotations present in the same aggregate index (i.e., class hierarchy level).

But if @TestPropertySource declared with different values on different levels (for example one in meta-annotation or parent class and another directly on class) than direct definition must override the value from meta-annotation or parent-class.

We have to be careful here when using the term "level", because there are in fact two types of levels: aggregate index and meta distance.

The current solution takes both types of levels into account. For a given aggregate index, all directly present and meta-present annotations at that level in the class hierarchy are sorted according to their meta distance, in reverse order. That effectively allows directly present annotations to override individual properties declared in directly present and meta-present annotations. But... that does not mean that a directly present annotation can use the semantics of inheritProperties = false to completely replace directly present or meta-present configuration. In other words, the inheritLocations and inheritProperties flags are not honored between annotations with the same aggregate index (i.e., at the same level in the test class hierarchy).

Perhaps I should explicitly document that last part in the Javadoc. 😉

[sbrannen]

Perhaps I should explicitly document that last part in the Javadoc. 😉

Addressed in 1954861

[philwebb]

I'd also appreciate feedback from you regarding use of the MergedAnnotations API in this PR (and the follow-up commits on the feature branch).

@sbrannen It all looks sensible to me. If you want to you could remove the synthesize() call, but I don't think it's a problem for tests. It's also possible to removing the comparator but you have to then work out when to append and when to prepend to the list. I tried that in this commit where I also moved some of the logic to TestPropertySourceAttributes. Feel free to cherry-pick some/all/none of it as you see fit.

[sbrannen]

Thanks, @philwebb.

The work for this feature has been refined in c9479ff and then further refined in 027fd78.