Skip to content
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.

Sign up for GitHub

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

Skip BWC tests in checkPart1 and checkPart2 #38730

Merged
merged 4 commits into from
Feb 14, 2019
Merged

Skip BWC tests in checkPart1 and checkPart2 #38730

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ea2a7f
Allow use of property to disable BWC tests.
alpar-t Feb 11, 2019
0b48099
Merge remote-tracking branch 'origin/master' into configurable-bwc-te…
alpar-t Feb 12, 2019
dda80fa
Skip BWC for checkPart1 and 2
alpar-t Feb 12, 2019
ddda603
Merge branch 'master' into configurable-bwc-test-enabled
alpar-t Feb 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ task verifyVersions {
* the enabled state of every bwc task. It should be set back to true
* after the backport of the backcompat code is complete.
*/
final boolean bwc_tests_enabled = true
boolean bwc_tests_enabled = true
final String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */
if (bwc_tests_enabled == false) {
if (bwc_tests_disabled_issue.isEmpty()) {
Expand All @@ -170,6 +170,17 @@ if (bwc_tests_enabled == false) {
println "See ${bwc_tests_disabled_issue}"
println "==========================================================="
}
if (project.gradle.startParameter.taskNames.find { it.startsWith("checkPart") } != null) {
Copy link
Contributor

@mark-vieira mark-vieira Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of thing tends to be quite brittle. One major issue is that "tasks that will execute" != "tasks you asked to execute". For example, if I introduce a task checkAllParts which dependsOn checkPart1, checkPart1 then this breaks down.

If we don't want tasks to run as part of a build we should define new tasks with the correct dependencies. Introducing logic like this into the build makes it very difficult to reason about why a given task was executed (or not) during a particular build invocation. In the end we should strive for more specific lifecycle tasks, not more dynamic logic. If I don't want task foo to run when I run checkPart, then checkPart shouldn't depend on foo. If making this so means adding more tasks and tweaking the existing dependency graph then we should do that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting the build up is not (yet) something we want to do, so we wouldn't want to embed this into every project. In the end, setting the bwc tests to false does prevent the dependencies to be added to check,
I'm not overly concerned about tasks that would depend on these partial checks as these are done specifically for PR checks in CI, maybe we should even reflect that in the name of the task to make it clear. I would be much more concerned if it would be possible to accidentally run fewer tests, running more than one intended isn't as bad.

// Disable BWC tests for checkPart* tasks as it's expected that this will run un it's own check
bwc_tests_enabled = false
}
if (project.gradle.startParameter.taskNames.contains("bwcTestSnapshots") && bwc_tests_enabled == false) {
throw new GradleException("BWC tests are disabled. " +
"This can happen if a branch happened to be created when they were disabled and can be solved by mergin at" +
"least to the commit on the parent branch that re-enabled them"
)
}

subprojects {
ext.bwc_tests_enabled = bwc_tests_enabled
}
Expand Down