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

[FIX JENKINS-49046] Fix WithTimeout handling for JenkinsRule #107

Merged
merged 3 commits into from Jul 19, 2018

Conversation

@olivergondza
Copy link
Member

olivergondza commented Jul 17, 2018

JENKINS-49046

Fixing JenkinsRule while keeping the deprecated HudsonTestCase as it is.

Copy link
Contributor

agentgonzo left a comment

This 'dirties' the JenkinsRUle configuration. ie, a @WithTimeout applied to a single test will change the timeout value in the JenkinsRule that will be applied to all subsequent tests - even those without any @WithTimeout annotation.

@@ -588,6 +590,14 @@ public void evaluate() throws Throwable {
}
}

private void applyTestTimeoutOverride(Description description) {
WithTimeout withTiemout = description.getAnnotation(WithTimeout.class);

This comment has been minimized.

Copy link
@agentgonzo

agentgonzo Jul 17, 2018

Contributor

typo: withTiemout

WithTimeout withTiemout = description.getAnnotation(WithTimeout.class);
if (withTiemout != null) {
timeout = withTiemout.value();
System.out.println("Using test timeout: " + timeout + " seconds");

This comment has been minimized.

Copy link
@agentgonzo

agentgonzo Jul 17, 2018

Contributor

Use a logging framework instead of System.out.println() (probably at the FINER or FINEST level)

@@ -153,4 +157,10 @@ public void run() {
}
}

@Test @WithTimeout(20)

This comment has been minimized.

Copy link
@agentgonzo

agentgonzo Jul 17, 2018

Contributor

This is needlessly long for a unit test (and will just make all test runs wait 20 seconds). You could easily make this @WithTimeout(1) and Thread.sleep(2000) to fulfill the same test criteria without waiting for 20 seconds on every test run.

This comment has been minimized.

Copy link
@olivergondza

olivergondza Jul 17, 2018

Author Member

Careful, this includes Jenkins startups time that rarely gets under 10s on my box. Timing out sooner would actually prevent the test code to be executed at all causing JUnit to consider it successful per the timeout exception thrown by the framework - not the body of the test.

This comment has been minimized.

Copy link
@agentgonzo

agentgonzo Jul 17, 2018

Contributor

Yeah, good point. Forgot it took into account the Jenkins startup time

@olivergondza

This comment has been minimized.

Copy link
Member Author

olivergondza commented Jul 17, 2018

This 'dirties' the JenkinsRUle configuration. ie, a @WithTimeout applied to a single test will change the timeout value in the JenkinsRule that will be applied to all subsequent tests - even those without any @WithTimeout annotation.

Note that this is how it used to be and it only applies to @ClassRules - @Rules get recreated for every individual test. The fix is reasonably straightforward, though, so I do not mind adding it.

@olivergondza

This comment has been minimized.

Copy link
Member Author

olivergondza commented Jul 17, 2018

@agentgonzo, thanks for the review. Comments either addressed or disputed.

@agentgonzo

This comment has been minimized.

Copy link
Contributor

agentgonzo commented Jul 17, 2018

Yeah, I was thinking about @ClassRules more specifically - I should have been more explicit.

@@ -153,4 +157,10 @@ public void run() {
}
}

@Test @WithTimeout(20)

This comment has been minimized.

Copy link
@agentgonzo

agentgonzo Jul 17, 2018

Contributor

Yeah, good point. Forgot it took into account the Jenkins startup time

@@ -588,6 +590,11 @@ public void evaluate() throws Throwable {
}
}

private int getTestTimeoutOverride(Description description, int def) {

This comment has been minimized.

Copy link
@agentgonzo

agentgonzo Jul 17, 2018

Contributor

Minor point. I would rename def to default to make it explicit. Also it could be confused with the def keyword in languages like groovy.

This comment has been minimized.

Copy link
@olivergondza

olivergondza Jul 17, 2018

Author Member

Heh, that is what I started with until compiler reminded me languages like Java has made default a keyword.

This comment has been minimized.

Copy link
@agentgonzo

agentgonzo Jul 17, 2018

Contributor

I don't even think you need to pass it into this method (as it's just read from this.timeout in the calling method anyway). You could remove the second parameter and read this.timeout internally in this method.

@jglick
jglick approved these changes Jul 19, 2018
@jglick jglick merged commit ffb00cc into jenkinsci:master Jul 19, 2018
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
@uhafner

This comment has been minimized.

Copy link
Member

uhafner commented Aug 24, 2018

Does this fix also handle the problems if JenkinsRule is used as ClassRule? In 1.39 version of the test harness tests using a ClassRule fail after 120 seconds for the whole suite. The WithTimeout annotation also seems to be only valid on a test case not on a test suite.

@olivergondza

This comment has been minimized.

Copy link
Member Author

olivergondza commented Sep 1, 2018

@uhafner, I do not know. Not where the problem was reproduced or unttested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.