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-50476] offer a way to assert that Jenkins won't start #96

Merged
merged 1 commit into from Apr 2, 2018

Conversation

batmat
Copy link
Member

@batmat batmat commented Mar 29, 2018

JENKINS-50476

Used in jenkinsci/jenkins#3364

@reviewbybees

@batmat batmat requested review from svanoort and jglick March 29, 2018 11:51
Copy link
Member

@svanoort svanoort left a comment

Choose a reason for hiding this comment

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

Looks fine to me, added a couple nits but nothing that is necessary to address.

if(entry.getValue()) {
throw e;
}
// Failure ignored as requested
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth logging something to indicate we saw expected failure, so we know rule ran okay.

try {
j.apply(step, description).evaluate();
if (!entry.getValue()) {
Assert.fail("The current JenkinsRule should have failed to start Jenkins.");
Copy link
Member

Choose a reason for hiding this comment

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

Super-🐜 - I like to phrase this as "X happened but we saw Y" because that makes it clearer.

/**
* List of {@link Statement}. For each one, the boolean value says if Jenkins is expected to start or not.
*/
private final Map<Statement, Boolean> steps = new LinkedHashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

🐜 I would declare the type here as explicitly LinkedHashmap since no other type could be legally used since we need ordering preserved.

What we're doing here is a bit dodgy (using a LinkedHashmap to simulate a List of Tuple types, with no intent to really use mapping)... but it's also much less verbose than the "proper" alternative and doesn't require adding Apache Commons Lang with a modern version to get tuple types.

@batmat batmat merged commit 15a06ec into jenkinsci:master Apr 2, 2018
@batmat batmat deleted the JENKINS-50476 branch April 2, 2018 07:01
@batmat
Copy link
Member Author

batmat commented Apr 2, 2018

@svanoort thanks a lot for the review. I will address your comments in a followup PR. I'm merging and releasing to make the associated core PR ready (instead of using a JTH SNAPSHOT).

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

Successfully merging this pull request may close these issues.

None yet

2 participants