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

Skip Flaky Java Windows Tests #1746

Merged
merged 18 commits into from
Jul 29, 2022
Merged

Skip Flaky Java Windows Tests #1746

merged 18 commits into from
Jul 29, 2022

Conversation

msaroufim
Copy link
Member

@msaroufim msaroufim commented Jul 20, 2022

Description

We have about ~14/192 tests that break constantly on Windows CI. There are probably more flaky ones we can fix in the future but at least with these fixed we can expect the Windows CI to turn green. The way to properly fix this would also involve mocking TS more so we don't actually have to run an inference.

We haven't had the bandwidth to fix the root cause for a few months so for now it may be best to skip and revisit as a better engineering task.

Not sure if this is the best way to skip tests using testng in Java so open to feedback

The CI report will clearly state which tests passed, failed or were skipped

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Feature/Issue validation/testing

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

Some tests that fail frequently are

@msaroufim msaroufim requested a review from agunapal July 20, 2022 01:20
@msaroufim msaroufim added bug Something isn't working windows labels Jul 20, 2022
@msaroufim
Copy link
Member Author

Found most of them!

Screen Shot 2022-07-20 at 2 46 32 PM

@rohithkrn
Copy link
Collaborator

In python, I know we can implement a common decorator like @skipIfWindows and annotate the tests. Not sure if there's a similar functionality in Java.

@msaroufim
Copy link
Member Author

In python, I know we can implement a common decorator like @skipIfWindows and annotate the tests. Not sure if there's a similar functionality in Java.

Oh yeah I love pytest where this kind of thing is easy

Even JUnit has a nice feature for it with @DisabledOnOs(WINDOWS) but this is the best I could find with testng

@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #1746 (adb69ee) into master (2d9c7cc) will not change coverage.
The diff coverage is n/a.

❗ Current head adb69ee differs from pull request most recent head 8584d43. Consider uploading reports for the commit 8584d43 to get more accurate results

@@           Coverage Diff           @@
##           master    #1746   +/-   ##
=======================================
  Coverage   45.38%   45.38%           
=======================================
  Files          64       64           
  Lines        2591     2591           
  Branches       60       60           
=======================================
  Hits         1176     1176           
  Misses       1415     1415           

Help us with your feedback. Take ten seconds to tell us how you rate us.

@msaroufim msaroufim requested a review from mreso July 29, 2022 18:37
Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

See comment

@@ -1133,7 +1134,11 @@ public void testModelWithInvalidCustomPythonDependency()
@Test(
alwaysRun = true,
dependsOnMethods = {"testModelWithInvalidCustomPythonDependency"})
public void testLoadingMemoryError() throws InterruptedException {
public void testLoadingMemoryError() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

throws allows multiple exception types to be reported. Is there a reason why you changed the type to "Exception" instead of "InterruptedException, SkipException"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip, yeah fixed that just now

Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

LGTM

@msaroufim
Copy link
Member Author

@rohithkrn @mreso we're all green

Screen Shot 2022-07-29 at 1 53 15 PM

@msaroufim msaroufim merged commit 967d8c7 into master Jul 29, 2022
@msaroufim msaroufim deleted the flakytestremoval branch July 29, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants