Set max-workers=1 for tests under CC #29169
Conversation
abstratt
force-pushed
the
rchaves/master/fix-flakiness-in-tests-under-cc
branch
from
df1254a
to
cc70a01
Compare
so they run deterministically, and we avoid flakiness.
abstratt
force-pushed
the
rchaves/master/fix-flakiness-in-tests-under-cc
branch
from
cc70a01
to
51d44ac
Compare
| @@ -27,6 +28,7 @@ class ConfigurationCacheGradleExecuter extends DaemonGradleExecuter { | |||
|
|
|||
| static final List<String> CONFIGURATION_CACHE_ARGS = [ | |||
| "--${ConfigurationCacheOption.LONG_OPTION}", | |||
| "-D${ParallelismBuildOptions.MaxWorkersOption.GRADLE_PROPERTY}=1", | |||
There was a problem hiding this comment.
🤔 I wonder if we lose some coverage here. I understand that a few tests depend on the order of tasks, though most of the tests shouldn't. And if we run those tests with more than one worker then we exercise some parallelism at runtime that we wouldn't do when setting this to only one worker. I am also quite sure that a few tests require more than one worker to work.
There was a problem hiding this comment.
My opinion on that is that if we want to test parallelism, we should test for that explicitly in tests dedicated for that. Everywhere else, concurrency adds non-determinism that is accidental to functionality being tested, making tests harder to write, or that are flaky.
Re: tests that require more than one worker to work, if they don't yet, they can override that option, right (and some already do)? There have been some failures in the first run, it may be due some instances of that.
There was a problem hiding this comment.
I think we want to test that parallelism introduced by CC doesn't break existing features. I don't think we need extra tests for all those cases.
There was a problem hiding this comment.
It is a tradeoff, for sure
There was a problem hiding this comment.
I am feeling increasingly strongly that we shouldn't do this. Instead of marking the tests that need parallelism, we should fix the tests that don't work with it. This would be an investment for the future when we enable CC by default.
There was a problem hiding this comment.
I agree with @wolfs. I feel it's important that new functionality / tests get exposure to the real world scenario of parallelism by default.
There was a problem hiding this comment.
I see accidental coverage as a secondary benefit, but addressing systemic flakiness that defeats test reliability would trump that.
I will archive this PR then.
There was a problem hiding this comment.
I understand your point of view but I wouldn't classify it as accidental coverage as these are E2E tests.
|
@bot-gradle test ACC |
|
I've triggered the following builds for you. Click here to see all build failures. |
|
@bot-gradle test ACC |
|
I've triggered the following builds for you. Click here to see all build failures. |
|
Will pursue a different approach. |
Please see: #29225 CC: @blindpirate |
so they run deterministically, and we avoid flakiness.
Reviewing cheatsheet
Before merging the PR, comments starting with