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

Disable two flaky tests (BEAM-8035, BEAM-9164) #11614

Merged
merged 1 commit into from May 6, 2020

Conversation

je-ik
Copy link
Contributor

@je-ik je-ik commented May 5, 2020

Disables two flaky tests until resolution is found.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- Build Status

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

Copy link
Contributor

@robertwb robertwb left a comment

Choose a reason for hiding this comment

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

Thank you.

@je-ik
Copy link
Contributor Author

je-ik commented May 5, 2020

@robertwb Thanks for approval, do we have a way of visualizing ignored tests? I'm a little afraid these test might get ignored for ever, which might be unfortunate.

@@ -242,6 +243,7 @@ public void close() {}
* <p>This test verifies that watermarks are correctly forwarded.
*/
@Test(timeout = 30_000)
@Ignore("https://issues.apache.org/jira/browse/BEAM-9164")
Copy link
Member

Choose a reason for hiding this comment

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

This one is apparently only flaky on Flink can we better exclude it manually only for Flink?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not @Category(ValidatesRunner) test, looks like it is Flink-specific already?

Copy link
Member

Choose a reason for hiding this comment

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

Oh my bad sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iemejia can I merge this? Is it fine for you?

Copy link
Member

Choose a reason for hiding this comment

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

Sure go ahead!

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO disabling tests should be the last resort. Please inform the mailing list or at least the JIRA issue when you disabled tests. This may be causing regressions. For example, I changed logic around UnboundedSourceWrapper a couple days ago and was not aware of tests being disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mxm I started a discussion thread here. Agree that we should have a better way of tracking flaky and disabled tests. There was also additional thread about precommit stability. The two tests disabled in this PR were causing practically permanent failure in precommits.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for starting the discussion. I'll try to enable these tests again. The recent changes in UnboundedSourceWrapper should have improved the test stability.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll run some tests here: #11679

@robertwb
Copy link
Contributor

robertwb commented May 5, 2020

I'm not sure we have more visibility into disabled tests than the jira entry (which shouldn't get closed until the tests are fixed and/or deemed unneeded and deleted.

@je-ik
Copy link
Contributor Author

je-ik commented May 5, 2020

@robertwb Looks like we should introduce some measures to solve this (not sure which measures these should be), because like we are accumulating these ignored tests:

~/git/apache/beam$ git grep "@Ignore" | wc -l
102

Most of associated issues remain open, which is what I would expect without better visibility. I'll start a discussion thread on ML.

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

4 participants