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.

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

Gradle Test runs should not be cacheable by default #9151

Closed
runningcode opened this issue Apr 17, 2019 · 6 comments
Closed

Gradle Test runs should not be cacheable by default #9151

runningcode opened this issue Apr 17, 2019 · 6 comments

Comments

@runningcode
Copy link
Contributor

@runningcode runningcode commented Apr 17, 2019

Tests, especially integration tests, may rely on eternal inputs such as time, date, external files, web resources. If the test runs themselves are cached, changes which cause the tests to fail will be hidden. Choosing to cache tests as the default choice is very dangerous because tests may pass when they should fail. Many projects do no keep up to date with release notes and this change may go unnoticed.

We have added the following workaround to our project to disable test caching:

tasks.withType(Test).configureEach {
  outputs.upToDateWhen { false }
}

Current Behavior:

Test task cacheability is on by default.

Proposed change:

Test task cacheability is off by default and opt in.

@runningcode runningcode changed the title Gradle Test Tasks should be opt-in cacheable. Gradle Test runs should not be cacheable by default Apr 17, 2019
@lptr lptr added the @build-cache label Apr 17, 2019
@realdadfish

This comment has been minimized.

Copy link

@realdadfish realdadfish commented Apr 17, 2019

While I agree that there should be an easy, visible configuration to disable test caching per module, I would not make test caching overall default to false. Reason is that especially in multi-module projects a lot of the module tests are pure, isolated JUnit tests that do not have to be run again if no code changes where detected in either the source or the test file sets. We depend for example on incremental builds and tests for our PR branches, where both, compilation and test avoidance, keeps the times for individual rebuilds low.

@tasomaniac

This comment has been minimized.

Copy link

@tasomaniac tasomaniac commented Apr 17, 2019

I agree. It's super valuable to cache them assuming that they are islolated (hermetic?) tests.

In case they are not hermetic (do network request), you can encapsulate all those tests in a separate module and use --rerun-tasks flag to fix your issue.

@runningcode

This comment has been minimized.

Copy link
Contributor Author

@runningcode runningcode commented Apr 17, 2019

It's a really common pattern to include files outside of the build source folder in integration tests.

For example: https://github.com/square/sqldelight/blob/master/sqldelight-gradle-plugin/src/test/kotlin/com/squareup/sqldelight/GradlePluginCombinationTests.kt

This test task does not declare the file dependencies.gradle as an input. Changes to dependencies.gradle like for example deleting versions.compileSdk would not cause the test to be re-run.

You raise a good point about build speeds. I guess it depends on what the priorities are for Gradle as a build system. While speed is extremely important for me as well, I would personally prefer a build system which favors correctness and reproducibility over speed. Dangerous potential speed improvements should be opt-in not opt-out.

@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Apr 17, 2019

This is true for any task that has some dynamic nature, even including the Java compiler (annotation processors could run arbitrary code). Also, this problem is not limited to caching. Not declaring your inputs will also break up-to-date checks. If we'd follow your line of reasoning, almost nothing would would be up-to-date or cacheable out of the box - a terrible first impression.

That's why when you do things outside of the box, you need to declare the additional inputs. This is really easy by using task.inputs. There's no need to do upToDateWhen {false}.

@oehme oehme closed this Apr 17, 2019
@lptr

This comment has been minimized.

Copy link
Member

@lptr lptr commented Apr 17, 2019

@runningcode it is possible to make that integration test cacheable and still get it re-executed when dependencies.gradle changes, you only have to declare it using Task.getInputs().file().

That said, it can be problematic not to forget to include extra inputs for integration tests. We are working on a plan to add something like a "verify mode" that would check/ensure that only files declared as inputs would be available to your tasks. In this case it would mean that your integration test would fail if you didn't include dependencies.gradle as its input.

@runningcode

This comment has been minimized.

Copy link
Contributor Author

@runningcode runningcode commented Apr 24, 2019

Closing in favor of #9210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.