Adjust Tooling API cross-version test ranges #29160
Conversation
donat
force-pushed
the
donat/fixit/tapi-test-ranges
branch
2 times, most recently
from
7cd8b9f
to
ed3501f
Compare
donat
requested review from
octylFractal and
a team
and removed request for
a team and
octylFractal
There was a problem hiding this comment.
This is really nice cleanup work!
If you want, I would value if you could make an attempt at maybe rephrasing the ToolingApiSpecification JavaDoc, so it has the:
- supported version ranges
- expresses when you need to use the annotations
so we would have less misuse of the annotations.This is really nice cleanup work!
If you want, I would value if you could make an attempt at maybe rephrasing theToolingApiSpecificationJavaDoc, so it has the: - supported version ranges
- expresses when you need to use the annotations
so we would have less misuse of the annotations.
| @ToolingApiVersion('<3.3 >=3.0') | ||
| @TargetGradleVersion('>=3.5') |
There was a problem hiding this comment.
❓Shouldn't this be in the accepted range?
There was a problem hiding this comment.
Especially @TargetGradleVersion('>=3.5'), as it's higher than 3.0
There was a problem hiding this comment.
We should only test TAPI 7+ versions. This test covers tapi versions [3.0, 3.3). So it should be safe to delete.
There was a problem hiding this comment.
Regarding your main comments, I don't want to duplicate information, but mentioning the rules in the docs would not hurt. I'll add something.
There was a problem hiding this comment.
At least point to the document
There was a problem hiding this comment.
We should only test TAPI 7+ versions.
But my main concern is about the Gradle version. Isn't the by-default @TargetGradleVersion('>=3.0')? Wouldn't that mean that we push the lower boundary of the Gradle version lower?
There was a problem hiding this comment.
If I understand your question correctly, you are asking whether the annotations combine or overwrite each other. It's the latter. If the base class has @TargetGradleVersion('>=4.0 <=6.0') and a test method has @TargetGradleVersion('>=5.0 <=7.0'), the method will be execute with Gradle versions 5.0, 6.0, 7.0 (and with the minor releases in between, of course).
There was a problem hiding this comment.
Added some documentation, ptal.
There was a problem hiding this comment.
you are asking whether the annotations combine or overwrite each other
here we go, probably this is why I didn't understand the situation!
...est/groovy/org/gradle/integtests/tooling/r213/ModelsWithGradleProjectCrossVersionSpec.groovy
Show resolved
Hide resolved
| // versions of Gradle. | ||
| @Ignore | ||
| @TargetGradleVersion("=2.10") | ||
| def "can use plugin built with current Gradle with old version"() { |
...sionTest/groovy/org/gradle/integtests/tooling/r35/RunTasksBeforeRunActionCrossVersion.groovy
Show resolved
Hide resolved
The Gradle documentation defines concrete rules on the supported Tooling API version ranges. To summarize
- A TAPI 8.9 client is compatible with Gradle versions [3.0, 10.0)
- The oldest TAPI client guaranteed to work with Gradle 8.9 is version 7.0.
These rules are captured by the @ToolingApiVersion and @TargetGradleVersion annotations on the ToolingApiSpecification class.
At the same time, many cross-version tests overwrite the test coverage version ranges with a more permissive rule. This leads to confusion and even flaky tests.
This change resolves the inconsistent version range declarations using the following rules:
- If a test has a more permissive version range than the default then the annotation is removed
- example: @ToolingApiVersion(">=5.4") is remove
- If the lower bound of the version range is lower then the default minimum then the lower bound is bumped to the minimum
- example: @TargetGradleVersion(">=2.6 <5.0") should be changed to @TargetGradleVersion(">=3.0 <5.0")
- If the version range is completely outside of the supported range then the entire test is removed
- example: @ToolingApiVersion(">=3.0 <4.0") or @TargetGradleVersion("=2.6")
donat
force-pushed
the
donat/fixit/tapi-test-ranges
branch
from
8c8520e
to
d50ba8d
Compare
|
@bot-gradle test RFN |
|
I've triggered the following builds for you. Click here to see all build failures. |
|
@bot-gradle merge |
The Gradle documentation defines concrete rules on the supported Tooling API version ranges. To summarize
These rules are captured by the
@ToolingApiVersionand@TargetGradleVersionannotations on the ToolingApiSpecification class.At the same time, many cross-version tests overwrite the test coverage version ranges with a more permissive rule. This leads to confusion and unexpected test failures as we can observe here.
This change resolves the inconsistent version range declarations using the following rules:
@ToolingApiVersion(">=5.4")is remove@TargetGradleVersion(">=2.6 <5.0")should be changed to @TargetGradleVersion(">=3.0 <5.0")@ToolingApiVersion(">=3.0 <4.0")or@TargetGradleVersion("=2.6")