-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix flaky GitSCMTest#testSubmoduleFixup
#1232
Fix flaky GitSCMTest#testSubmoduleFixup
#1232
Conversation
The test should wait for the build to complete before actually checking its status. ``` java.lang.AssertionError: unexpected build status; build log was: ------ Started by upstream project "test0" build number 1 originally caused by: Legacy code started this job. No cause information is available Running as SYSTEM Building in workspace /jenkins/workspace/builder/output-git/work/git/target/tmp/j h69583182000798596/workspace/test1 The recommended git tool is: NONE No credentials specified Cloning the remote Git repository Cloning repository /jenkins/workspace/builder/output-git/work/git/target/tmp/junit3569465322501079491/junit1849114342070567213 > git init /jenkins/workspace/builder/output-git/work/git/target/tmp/j h69583182000798596/workspace/test1 # timeout=10 ------ Expected: is <SUCCESS> but: was null at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20) at org.junit.Assert.assertThat(Assert.java:964) at org.jvnet.hudson.test.JenkinsRule.assertBuildStatus(JenkinsRule.java:1439) at org.jvnet.hudson.test.JenkinsRule.assertBuildStatusSuccess(JenkinsRule.java:1468) at hudson.plugins.git.GitSCMTest.testSubmoduleFixup(GitSCMTest.java:1675) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54) at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54) at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:601) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.lang.Thread.run(Thread.java:748) ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for detecting that mistake in the test.
if (isWindows()) { | ||
return; | ||
} | ||
Assume.assumeFalse(isWindows()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
These tests intentionally use the if
construct rather than the Assume
construct because the Assume
construct generates a warning about skipped tests that then is shown as a warning in the warnings ng plugin output. I'd prefer to continue using the early exit without using a JUnit construct so that the warning is not generated.
Alternately, if the waitForCompletion
resolves the flakiness in the test, then let's remove the early return and let the test run on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started a test series on my infra to see if the Assume
can be removed for Windows. Will report the results here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposing two changes to check the behavior on ci.jenkins.io
Change from @Vlatombe fixed unreliability of test on Windows also.
The test should wait for the build to complete before actually checking
its status.
Fix flaky
GitSCMTest#testSubmoduleFixup
Wait for downstream build completion before checking its status.
Checklist
Types of changes