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

failOnPassedAfterRetry should not be a part of the cache key #113

Open
runningcode opened this issue Jul 22, 2021 · 3 comments
Open

failOnPassedAfterRetry should not be a part of the cache key #113

runningcode opened this issue Jul 22, 2021 · 3 comments
Labels
enhancement New feature or request

Comments

@runningcode
Copy link
Contributor

Many projects have a different value set for failOnPassedAfterRetry locally vs on CI systems. This will cause a remote cache miss for test tasks.

Since this property causes a test task to fail in more cases and thus not generate a cache entry it would mean that the successful outcomes of the test task are the same, its just a smaller set of cases where a cache entry is created.

Because of this, would it be possible to remove this flag as an input to the task and thus avoid this cache miss?

@bigdaz
Copy link
Member

bigdaz commented Jul 22, 2021

The problem with removing the flag as an input would be the following case:

  • build1 runs with failOnPassedAfterRetry=false. The test task passes after some tests required a retry. Success is written to cache.
  • build2 runs with failOnPassedAfterRetry=true. This task uses the cached output from build1 and the test task passes when it should have failed.

Consider a hypothetical test that always fails on the first invocation and always passes on retry. The test task on build2 should never pass, but with the proposed change it could pass when the result is taken from the cache.

The real solution would be to consider a cache entry from build2 to satisfy build1, but not vice versa.

Something similar could be used for a task that runs a subset of tests: if there's a cache entry where all tests pass, then we could use it to avoid running a subset of tests. But we would never want to do the opposite.

@runningcode
Copy link
Contributor Author

That makes sense. For an ideal world we would only want caching in one direction. But in practice, I think the outcomes and successful task executions that are cached are the same. These are flaky tests. We are just caching the result when they're not flaky. This would be same as if the test happend to execute successfully the first time.

Another thing to consider is that the maxRetries property is not considered an input.

@runningcode
Copy link
Contributor Author

For those who experience this miss but one of the two builds has maxRetries set to 0 setting failOnPassedAfterRetry = true on both CI and local builds will have the same effect. That is since there are no retries for local builds the pass / failure behavior is the same.

runningcode added a commit that referenced this issue Sep 29, 2021
Also reduce the number of suggested retries to 2.

Ref #113
runningcode added a commit that referenced this issue Sep 29, 2021
Also reduce the number of suggested retries to 2.

Ref #113
runningcode added a commit that referenced this issue Sep 29, 2021
Also reduce the number of suggested retries to 2.

Ref #113

Signed-off-by: Nelson Osacky <[email protected]>
@pshevche pshevche added the enhancement New feature or request label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants