-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Enable IP tests for :core
#29202
Enable IP tests for :core
#29202
Conversation
1fd897b
to
ee8cbb3
Compare
@@ -39,7 +40,10 @@ class ConfigurationOnDemandIntegrationTest extends AbstractIntegrationSpec { | |||
file("gradle.properties") << "org.gradle.configureondemand=true" | |||
} | |||
|
|||
@Requires(value = IntegTestPreconditions.NotParallelExecutor, reason = "parallel mode hides incubating message") | |||
@Requires( | |||
value = [IntegTestPreconditions.NotParallelExecutor, IntegTestPreconditions.NotIsolatedProjects], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Since Isolated Projects implies Configuration Cache implies parallel execution, shouldn't NotParallelExecutor
be enough here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically speaking, CC executor doesn't imply parallel executor (and was never implying). See
Line 86 in 3c1f32c
public static boolean isConfigCache() { |
CC <-> parallelism relation is being expressed like a
Line 112 in 6a39fc7
static final class NotParallelOrConfigCacheExecutor implements TestPrecondition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Thanks for the pointers, @6hundreds. I'm now thinking, why shouldn't we update this definition to configCache(true, true)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that in separate PR
Oh, yeah, didn't mean otherwise 👍
@@ -76,6 +83,7 @@ class ConfigurationOnDemandIntegrationTest extends AbstractIntegrationSpec { | |||
fixture.assertProjectsConfigured(":") | |||
} | |||
|
|||
@ToBeFixedForIsolatedProjects(because = "Allprojects") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 Let's preserve the incompatible API spelling as allprojects for easier searching, just in case
@@ -378,6 +390,7 @@ project(':api') { | |||
fixture.assertProjectsConfigured(":", ":b", ":a") | |||
} | |||
|
|||
@ToBeFixedForIsolatedProjects(because = "Configure projects from root, buildDependents is not IP compatible") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 Probably better to always keep the API name closer to the because = "
for easier search
@@ -378,6 +390,7 @@ project(':api') { | |||
fixture.assertProjectsConfigured(":", ":b", ":a") | |||
} | |||
|
|||
@ToBeFixedForIsolatedProjects(because = "Configure projects from root, buildDependents is not IP compatible") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ToBeFixedForIsolatedProjects(because = "Configure projects from root, buildDependents is not IP compatible") | |
@ToBeFixedForIsolatedProjects(because = "buildDependents is not IP compatible, configure projects from root") |
@@ -150,6 +151,7 @@ class UserInputHandlingIntegrationTest extends AbstractUserInputHandlerIntegrati | |||
"n" | _ | |||
} | |||
|
|||
@ToBeFixedForIsolatedProjects(because = "Subprojects") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ToBeFixedForIsolatedProjects(because = "Subprojects") | |
@ToBeFixedForIsolatedProjects(because = "subprojects") |
|
||
class IsolatedProjectsSettingsBuildOperationsIntegrationTest extends AbstractIntegrationSpec { | ||
|
||
def operations = new BuildOperationsFixture(executer, temporaryFolder) | ||
|
||
@Requires( | ||
value = IntegTestPreconditions.NotIsolatedProjects, | ||
reason = "Exercises disabled IP behavior" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ What is the disabled behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad wording apparently, sorry. It's better to say that the test is literally controls enablement of IP by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest moving the annotation to the class level.
In general, I would suspect test classes starting with IsolatedProjects*
are either using a corresponding fixture to control the feature, or expect it to not be enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, moved to the top class level
@@ -146,6 +147,7 @@ class ParallelTaskExecutionIntegrationTest extends AbstractIntegrationSpec imple | |||
} | |||
} | |||
|
|||
@ToBeFixedForIsolatedProjects(because = "Allprojects in setup") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 When there is a violation in the setup, would it make sense to put the annotation also on the test class level?
We don't have to change it here, since the work has already been done, but it may be helpful for the annotating other Gradle modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
Now ToBeFixedWithIsolatedProjects
is not supposing for type application. It'll require some improvements in the spec extension, so I would plan and do that in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
782a01e
to
659c741
Compare
@@ -34,6 +34,7 @@ class ConcurrentArchiveIntegrationTest extends AbstractIntegrationSpec { | |||
BlockingHttpServer server = new BlockingHttpServer() | |||
|
|||
@Issue("https://github.com/gradle/gradle/issues/22685") | |||
@ToBeFixedForConfigurationCache(because = "Access to root project from sub projects") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Configuration Cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooops, my bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Part of #29045
This PR is only annotating tests as
to-be-fixed-for-ip
/incompatible-with-ip
and doesn't try to fix them. Although some tests could be fixed easily, this is going to be done in separate PR