Do not evaluate feature-skipping conditions for skipped specs#1459
Merged
Conversation
Verify that the feature also works for hierarchies of more than 2 classes, i.e. there are other classes in between annotated spec and bottom spec. Also verify that it still works correctly if the parent specs are non-abstract, i.e. that they are ignored correctly not because of being abstract but because of the corresponding annotation. The parent classes also have their own feature methods now, so we can do a more sophisticated count of executed/skipped features and containers.
Codecov Report
@@ Coverage Diff @@
## master #1459 +/- ##
=============================================
+ Coverage 0 79.59% +79.59%
- Complexity 0 4015 +4015
=============================================
Files 0 404 +404
Lines 0 12565 +12565
Branches 0 1640 +1640
=============================================
+ Hits 0 10001 +10001
- Misses 0 1969 +1969
- Partials 0 595 +595
Continue to review full report at Codecov.
|
The new spec MixedSpecSkippingExtensions verifies that this works correctly for both inherited and non-inherited versions of @requires, @IgnoreIf and - not to forget - @ignore. Because the two feature methods verify combinations of annotations, they did not fit into any of the existing specs focused on single annotation-driven extensions. A separate spec makes more sense. It verifies both the new feature as such and interoperability between spec-ignoring extension types. Fixes spockframework#1458.
Raise the bar in MixedSpecSkippingExtensions, also verifying that feature-skipping annotation conditions are not evaluated in skipped parent classes of the bottom spec, up to the actually annotated, skipped spec. Before this change, the spec-skipping extensions applied a premature optimisation I was skeptical of before: to only mark the bottom spec as skipped for an inherited annotation, instead of marking everything down to the bottom spec, starting with the annotated spec. This is the proper way to do it, if we are talking about inheritance. In order to achieve this more easily, I added new utility methods 'SpecInfo.getSpecsToBottom()' and 'SpecInfo.getSpecsToTop()'. The latter is unused at the moment, but might come in handy in the future. Fixes spockframework#1458 (really, this time).
This both helps to better understand what they do and also provides a bit more code coverage.
This has implications for the number of started, skipped, succeeding and failing tests and containers. We want to make sure the numbers are reported correctly, looking up the inheritance hierarchy, not just down. This helps to spot possible regressions where accidentally not just sub specs, but also or instead super specs would be skipped.
leonard84
requested changes
Apr 27, 2022
Member
leonard84
left a comment
leonard84
left a comment
There was a problem hiding this comment.
Thanks for the PR this is going to be a valuable improvement.
The others follow later, if applicable. Co-authored-by: Leonard Brünings <lord_damokles@gmx.net>
leonard84
requested changes
May 4, 2022
leonard84
reviewed
May 4, 2022
* master: Bump org.gradle.test-retry from 1.3.2 to 1.4.0 Make EnableSharedInjection public (spockframework#1472) Remove runtime dependency on Jetbrains Annotations (spockframework#1468) Add Dependabot configuration Use static imports for well known methods of Collections and Arrays Refactor SpecInfo extract shared logic into collectSpecHierarchy Discard unnecessary state in ConfineMetaClassChangesInterceptor (spockframework#1460) Update Groovy to 4.0.2 Update Gradle plugins Update Gradle to 7.4.2
leonard84
approved these changes
May 21, 2022
Member
|
Thanks @kriegaex |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The new spec
MixedSpecSkippingExtensionsverifies that this works correctly for both inherited and non-inherited versions of@Requires,@IgnoreIfand - not to forget -@Ignore. Because the two feature methods verify combinations of annotation types, they did not fit into any of the existing specs focused on single annotation-driven extensions. A separate spec makes more sense. It verifies both the new feature as such and interoperability between spec-ignoring extension types.Fixes #1458.
In an additional commit, I raised the bar a bit, also verifying that feature-skipping annotation conditions are not evaluated in skipped parent classes of the bottom spec, up to the actually annotated, skipped spec.
Before this change, the spec-skipping extensions applied a premature optimisation I was skeptical of before: to only mark the bottom spec as skipped for an inherited annotation, instead of marking everything down to the bottom spec, starting with the annotated spec. This is the proper way to do it, if we are talking about inheritance.
In order to achieve this more easily, I added new utility methods
SpecInfo.getSpecsToBottom()andSpecInfo.getSpecsToTop(). The latter is unused at the moment, but might come in handy in the future.