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

Investigate reasons behind TestTools.allowFailure #572

Closed
devinrsmith opened this issue May 10, 2021 · 3 comments
Closed

Investigate reasons behind TestTools.allowFailure #572

devinrsmith opened this issue May 10, 2021 · 3 comments
Assignees
Milestone

Comments

@devinrsmith
Copy link
Member

May explain some CI shortcomings. May be related to @niloc132 s frustrations in #559 . Potentially working around issues that we don't need to work around if this was removed. See #560

@niloc132
Copy link
Member

The property allowFailure drives this, but unless explicitly set to "do not allow a failure" with a value of false, it will always allow failures. This is the default, that developers and CI alike will experience, unless they have explicitly set this.

Going by the root build.gradle, it looks like this was added to protect against flaky tests - the config Test.ignoreFailures is set (letting a failed test not fail the build), and then if set, the test task also gets some extra wiring - make sure that tests can re-run (prevent caching from "helping", count the failures as they happen, and log a message indicating failure at the end of the build, even though the build cannot fail any more due only to tests.

Two approaches I see that we can take from here:

  • Assume that tests are currently consistent: remove this flag, or change the default so that it is only permitting failures in specific circumstances. This is my preferred option, provided we think tests are actually not failing spuriously at this time.
  • Assume that tests may have spurious failures: apply the same "build success even when tests fail" behavior to other tests like python. This is not historically what Venv has done - when --continue is set, a failing py test would fail the build, without an extra. This inconsistency causes confusion when trying to set up CI - we seem to be past it for now, but it is hard to be sure we've handled all cases when we don't deal with the cases consistently...

@niloc132
Copy link
Member

#575 is blocking cleaning this up.

devinrsmith added a commit to devinrsmith/deephaven-core that referenced this issue May 12, 2021
@devinrsmith
Copy link
Member Author

https://discuss.gradle.org/t/prevent-caching-of-test-tasks-with-failures/39842/2 seems to be a similar reason why we've done it, and the pitfalls we've faced. I've got a test PR that removes setting ignoreFailures #588 , going through some CI runs ATM to see how it acts.

devinrsmith added a commit to devinrsmith/deephaven-core that referenced this issue May 12, 2021
@pete-petey pete-petey modified the milestones: Backlog, May 2021 May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants