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

[JENKINS-28840] Cancel the test timer only after we call Jenkins.cleanUp, in case that hung #1809

Closed
wants to merge 2 commits into from

Conversation

@jglick
Copy link
Member

jglick commented Aug 20, 2015

Might have rendered changes like jenkinsci/pipeline-plugin#184 unnecessary by making the test fail cleanly even if it hits a deadlock like JENKINS-28840; though in that case it is not clear to me that even interrupting the cleanUp thread would help, since Queue._withLock is using lock not lockInterruptibly. Left in a test case for a future improvement.

(BTW @lordofthejars @kohsuke we should consider extracting jenkinsci/jenkins/tree/master/test/src/main/ to a separate repository with its own release cycle where we can deliver fixes more quickly without being tied down to the core release cycle.)

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Aug 20, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

}
}

@Ignore("TODO ought to use the idiom from org.junit.internal.runners.statements.FailOnTimeout: run test in its own thread so we can fail even if it does not respond to interruptions")

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 20, 2015

Member

what is the width? Do you have any line in your IDE?

This comment has been minimized.

Copy link
@jglick

jglick Aug 20, 2015

Author Member

My IDE has a line at 80 characters, which I ignore, because my screen can show more than 80 characters on a line, and anyway it wraps lines longer than that, so who cares?

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 20, 2015

Member

@jglick those who are reading this PR on GH enforced to scroll. //cc @lanwen
Should be not so difficult to fit in 120-130 lines

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 20, 2015

Member

@jglick Please add line breaks to limit line length to 120 symbols. It very hard to read and scroll such loooong lines. It's one of the most popular oss project, so it should be compatible with oracle code style conventions as most popular in java world.

Also please don't inline @Test annotations, it makes me cry when I see it.

@Test public void hangInterruptiblyInShutdown() throws Exception {
System.err.println("Test itself passed…");
}
@TestExtension("hangInterruptiblyInShutdown") public static class HangsInterruptibly extends ItemListener {

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 20, 2015

Member

This is disgusting readable, but you can continue if you planned this code write-only.

This comment has been minimized.

Copy link
@lordofthejars

lordofthejars Aug 20, 2015

I don't care if annotations are on the same line or not, but if we choose to use inline approach then everywhere in the project should be inline.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 21, 2015

Member

This is Jesse's style.
Inline annotations are useful sometimes IMO (e.g. method return values), but for big classes they look to be strange


@Rule public JenkinsRule r = new JenkinsRule();
{
r.timeout = 10; // let us not wait three minutes!

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 20, 2015

Member

Timeout annotation can be used as more verbose

This comment has been minimized.

Copy link
@jglick

jglick Aug 21, 2015

Author Member

Not sure I understand the comment. The default JenkinsRule.timeout is 180. I am reducing that in this test because I specifically expect to hit it, and do not want the test to be unreasonably slow.

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 23, 2015

Member

There is annotation org.jvnet.hudson.test.recipes.WithTimeout which is more readable.

@lordofthejars

This comment has been minimized.

Copy link

lordofthejars commented Aug 20, 2015

I completely agree that would be really useful and more manageable to have
this project out of Jenkins core project and move to its own repo and
artifact. Next week probably I have a meeting with @kohsuke and we can talk about this too.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 21, 2015

🐝 for the change, I leave code formatting holywars to others

…een attribute and determiner annotations, to satisfy arbitrary reviewer demands.
public void hangInterruptiblyInShutdown() throws Exception {
System.err.println("Test itself passed…");
}
@TestExtension("hangInterruptiblyInShutdown")

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 24, 2015

Member

NIT: missing space

This comment has been minimized.

Copy link
@jglick

jglick Aug 24, 2015

Author Member

Intentional, since the extension applies only to the method directly above, and Java offers no way of making such scopes explicit.

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 24, 2015

Member

maybe separate previous block with comments // Test extension?

This comment has been minimized.

Copy link
@slide

slide Aug 24, 2015

Member

Why when the @TestExtension is pretty clear already?

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 24, 2015

Member

@slide @jglick mean that this previous function is used only for TestExtension, but as i see it a separate test, so no need in skipping new lines (imho)

@lanwen

This comment has been minimized.

Copy link
Member

lanwen commented Aug 24, 2015

@jglick woohooo, thx!

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 24, 2015

🐝

@KostyaSha

This comment has been minimized.

Copy link
Member

KostyaSha commented Aug 24, 2015

👍 if sqash

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Aug 24, 2015

🐝

jglick added a commit that referenced this pull request Aug 24, 2015
@jglick jglick closed this Aug 24, 2015
@jglick jglick deleted the jglick:JenkinsRule-timeout branch Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.