GH-1438 - naive implementation to skip all tests if configuration on-no-changes=execute-none is set #1484
Conversation
|
|
||
| if (!changes.hasClassChanges()) { | ||
| return enabled("No source file changes detected."); | ||
| var environment = new StandardEnvironment(); |
There was a problem hiding this comment.
would it make sense to adjust ConditionContext so that here is no need to create StandardEnvironment?
or in another way so that also StateStore can profit from a simplification (there is also an instantiation of StandardEnvironment)?
also a test is missing. im not sure how you prefer to structure the test setup, particularly regarding setting the value in application.yml to skip all tests when no class changes are detected.
There was a problem hiding this comment.
We should use StateStore.getChanges() to look up the new property and massage its presence into a Change.NONE_AND_PROCEED or Change.NONE_AND_SKIP constant. We need the StandardEnvironment processing there anyway to read other properties from Boot standard locations.
I've just pushed a change to main to slightly refactor the code a bit so that the creation of a Changes instance now depends solely on an Environment. I've added a unit test for the existing property lookup in ChangesFactoryUnitTests. You should be able to add tests there more easily.
| var environment = new StandardEnvironment(); | ||
| ConfigDataEnvironmentPostProcessor.applyTo(environment); | ||
|
|
||
| return "execute-none".equals(environment.getProperty("spring.modulith.test.on-no-changes")) |
There was a problem hiding this comment.
a switch expression could be used here. ("execute-none", "execute-all")
but i would actually prefer a boolean but a renaming of "spring.modulith.test.on-no-changes" would then make sense like "spring.modulith.test.skip-all-on-no-class-changes"
There was a problem hiding this comment.
Let's stick with an enum here, please. True/false hardly make any sense without context, especially when handed as parameters into methods. The enum can be a nested type of Changes.
There was a problem hiding this comment.
enum sounds great. i just dont know if i understand it correctly.
i pushed an attempt.
…nfiguration on-no-changes=execute-none is set
|
in |
|
|
||
| if (!changes.hasClassChanges()) { | ||
| return enabled("No source file changes detected."); | ||
| var environment = new StandardEnvironment(); |
There was a problem hiding this comment.
We should use StateStore.getChanges() to look up the new property and massage its presence into a Change.NONE_AND_PROCEED or Change.NONE_AND_SKIP constant. We need the StandardEnvironment processing there anyway to read other properties from Boot standard locations.
I've just pushed a change to main to slightly refactor the code a bit so that the creation of a Changes instance now depends solely on an Environment. I've added a unit test for the existing property lookup in ChangesFactoryUnitTests. You should be able to add tests there more easily.
| var environment = new StandardEnvironment(); | ||
| ConfigDataEnvironmentPostProcessor.applyTo(environment); | ||
|
|
||
| return "execute-none".equals(environment.getProperty("spring.modulith.test.on-no-changes")) |
There was a problem hiding this comment.
Let's stick with an enum here, please. True/false hardly make any sense without context, especially when handed as parameters into methods. The enum can be a nested type of Changes.
spring-modulith-junit/src/main/java/org/springframework/modulith/junit/Changes.java
Outdated
Show resolved
Hide resolved
spring-modulith-junit/src/main/java/org/springframework/modulith/junit/Changes.java
Outdated
Show resolved
Hide resolved
...-modulith-junit/src/main/java/org/springframework/modulith/junit/TestExecutionCondition.java
Outdated
Show resolved
Hide resolved
| }, | ||
| { | ||
| "name": "spring.modulith.test.on-no-changes", | ||
| "type": "java.lang.String", |
There was a problem hiding this comment.
I think we can express an enum here directly. I just don't know how from the top of my head. I solely used hints in the other cases, as with those, we both support well-known values and custom strings.
There was a problem hiding this comment.
neither do i. i did searching in spring boot repo an example but after some time i gave up.
i pushed a new version.
|
Thank you very much. I'll take it from here. |
Original pull request: GH-1484.
Original pull request: GH-1484.
|
That's polished and merged. Thanks! |
Original pull request: GH-1484. Signed-off-by: Valentin Bossi <[email protected]>
Original pull request: GH-1484.
2 participants
resolves #1438