Skip to content

Commit ddd190b

Browse files
jan-molakjan-molak
committed
The list of culprits is no longer accumulated from all the failed builds, as slow Jenkins username lookups had significant performance implications if the build remained broken for a long period of time and there was a large number of commits made over such a broken build.
Even though this scenario is a typical Continuous Integration anti-pattern, it's important that Build Monitor remains fast no matter how people use it :-) The new behaviour works as follows: - if what broke the build was a code change - display the authors - if there were other commits made since that first unlucky one - display the author(s) of the first commit and the number of builds broken since Fixes jenkinsci#138, fixes jenkinsci#151
1 parent 22c6ca4 commit ddd190b

File tree

12 files changed

+238
-93
lines changed

12 files changed

+238
-93
lines changed

‎pom.xml

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
<groupId>org.jenkins-ci.plugins</groupId>
1111
<artifactId>build-monitor-plugin</artifactId>
12-
<version>1.6-SNAPSHOT</version>
12+
<version>1.7-SNAPSHOT</version>
1313
<packaging>hpi</packaging>
1414

1515
<name>Build Monitor View</name>

‎src/main/java/com/smartcodeltd/jenkinsci/plugins/buildmonitor/readability/Lister.java

+59
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package com.smartcodeltd.jenkinsci.plugins.buildmonitor.readability;
2+
3+
import java.util.List;
4+
5+
import static java.lang.String.format;
6+
7+
public class Lister {
8+
private static final String DEFAULT_NO_ITEMS_TEMPLATE = "%s";
9+
10+
public static <T> String describe(String pluralTemplate, List<T> items) {
11+
return describe(DEFAULT_NO_ITEMS_TEMPLATE, pluralTemplate, items);
12+
}
13+
14+
public static <T> String describe(String noItemsTemplate, String pluralTemplate, List<T> items) {
15+
return describe(noItemsTemplate, pluralTemplate, pluralTemplate, items);
16+
}
17+
18+
public static <T> String describe(String noItemsTemplate, String singularTemplate, String pluralTemplate, List<T> items) {
19+
switch (items.size()) {
20+
case 0: return formatted(noItemsTemplate, items);
21+
case 1: return formatted(singularTemplate, items);
22+
default: return formatted(pluralTemplate, items);
23+
}
24+
}
25+
26+
public static <T> String asString(List<T> items) {
27+
return items.isEmpty() ? "" : asString(headOf(items), restOf(items));
28+
}
29+
30+
// --
31+
32+
private static <T> String formatted(String template, List<T> items) {
33+
return format(template, asString(items));
34+
}
35+
36+
private static <T> String asString(String acc, List<T> tail) {
37+
switch(tail.size()) {
38+
case 0: return acc;
39+
case 1: return and(acc, headOf(tail));
40+
default: return asString(comma(acc, headOf(tail)), restOf(tail));
41+
}
42+
}
43+
44+
private static String and(String first, String second) {
45+
return format("%s and %s", first, second);
46+
}
47+
48+
private static String comma(String first, String second) {
49+
return format("%s, %s", first, second);
50+
}
51+
52+
private static <T> String headOf(List<T> items) {
53+
return items.get(0).toString();
54+
}
55+
56+
private static <T> List<T> restOf(List<T> items) {
57+
return items.subList(1, items.size());
58+
}
59+
}

‎src/main/java/com/smartcodeltd/jenkinsci/plugins/buildmonitor/readability/Pluraliser.java

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
package com.smartcodeltd.jenkinsci.plugins.buildmonitor.readability;
2+
3+
import static java.lang.String.format;
4+
5+
public class Pluraliser {
6+
public static String pluralise(String template, int count) {
7+
return format(template, count);
8+
}
9+
10+
public static String pluralise(String singularTemplate, String pluralTemplate, int count) {
11+
return count == 1
12+
? pluralise(singularTemplate, count)
13+
: pluralise(pluralTemplate, count);
14+
}
15+
16+
public static String pluralise(String zeroCountTemplate, String singularTemplate, String pluralTemplate, int count) {
17+
return count == 0
18+
? pluralise(zeroCountTemplate, count)
19+
: pluralise(singularTemplate, pluralTemplate, count);
20+
}
21+
}

‎src/main/java/com/smartcodeltd/jenkinsci/plugins/buildmonitor/viewmodel/JobView.java

+41-11
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
package com.smartcodeltd.jenkinsci.plugins.buildmonitor.viewmodel;
22

3+
import com.google.common.collect.Lists;
34
import com.smartcodeltd.jenkinsci.plugins.buildmonitor.Config;
45
import com.smartcodeltd.jenkinsci.plugins.buildmonitor.facade.RelativeLocation;
6+
import com.smartcodeltd.jenkinsci.plugins.buildmonitor.readability.Lister;
7+
import com.smartcodeltd.jenkinsci.plugins.buildmonitor.readability.Pluraliser;
58
import com.smartcodeltd.jenkinsci.plugins.buildmonitor.viewmodel.duration.Duration;
69
import com.smartcodeltd.jenkinsci.plugins.buildmonitor.viewmodel.plugins.BuildAugmentor;
710
import hudson.model.Job;
@@ -10,10 +13,10 @@
1013
import org.codehaus.jackson.annotate.JsonProperty;
1114

1215
import java.util.Date;
13-
import java.util.HashSet;
1416
import java.util.List;
15-
import java.util.Set;
1617

18+
import static com.google.common.collect.Lists.newLinkedList;
19+
import static com.google.common.collect.Lists.reverse;
1720
import static hudson.model.Result.SUCCESS;
1821

1922
/**
@@ -94,27 +97,54 @@ public int progress() {
9497
}
9598

9699
@JsonProperty
97-
public boolean shouldIndicateCulprits() {
98-
return ! isClaimed() && culprits().size() > 0;
100+
public String headline() {
101+
// todo: extract and clean up
102+
List<BuildViewModel> failedBuildsAsc = reverse(failedBuilds());
103+
104+
switch(failedBuildsAsc.size()) {
105+
case 0:
106+
return "";
107+
108+
case 1:
109+
return Lister.describe(
110+
"",
111+
"Failed after %s committed their changes",
112+
newLinkedList(failedBuildsAsc.get(0).culprits())
113+
);
114+
115+
default:
116+
String buildsFailedSince = Pluraliser.pluralise(
117+
"%s build has failed",
118+
"%s builds have failed",
119+
failedBuildsAsc.size() - 1
120+
);
121+
122+
return Lister.describe(
123+
buildsFailedSince,
124+
buildsFailedSince + " since %s committed their changes",
125+
newLinkedList(failedBuildsAsc.get(0).culprits())
126+
);
127+
}
99128
}
100129

101-
@JsonProperty
102-
public Set<String> culprits() {
103-
Set<String> culprits = new HashSet<String>();
130+
private List<BuildViewModel> failedBuilds() {
131+
List<BuildViewModel> failedBuilds = Lists.newArrayList();
104132

105133
BuildViewModel build = lastBuild();
106-
// todo: consider introducing a BuildResultJudge to keep this logic in one place
107134
while (! SUCCESS.equals(build.result())) {
108-
culprits.addAll(build.culprits());
135+
136+
if (! build.isRunning()) {
137+
failedBuilds.add(build);
138+
}
109139

110140
if (! build.hasPreviousBuild()) {
111141
break;
112142
}
113143

114144
build = build.previousBuild();
115-
};
145+
}
116146

117-
return culprits;
147+
return failedBuilds;
118148
}
119149

120150
public boolean isDisabled() {

‎src/main/resources/com/smartcodeltd/jenkinsci/plugins/buildmonitor/BuildMonitorView/index.jelly

+2-1
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,8 @@
100100
<footer>
101101
<notifier />
102102
<span class="plugin-author">
103-
Build Monitor <a href="https://github.com/jan-molak/jenkins-build-monitor-plugin/releases" title="Is your Build Monitor up to date?" rel="external">${it.installation.buildMonitorVersion()}</a>
103+
<a href="https://github.com/jan-molak/jenkins-build-monitor-plugin" title="Build Monitor project" rel="external">Build Monitor</a>
104+
version <a href="https://github.com/jan-molak/jenkins-build-monitor-plugin/releases" title="Is your Build Monitor up to date?" rel="external">${it.installation.buildMonitorVersion()}</a>
104105
brought to you by <a href="http://smartcodeltd.co.uk" rel="external">Jan Molak</a>
105106
</span>
106107
</footer>

‎src/main/resources/com/smartcodeltd/jenkinsci/plugins/buildmonitor/BuildMonitorView/main-jobViews.jelly

+6-4
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@
1414
<a title="{{job.name}}"
1515
href="{{job.url}}">{{job.name}}</a>
1616
</h2>
17-
<ul class="description">
18-
<li data-ng-show="job.claimed">Claimed by <strong>{{ job.claimAuthor }}</strong>: {{ job.claimReason }}</li>
17+
<ul class="description" data-ng-show="!!job.claimed">
18+
<li>Claimed by <strong>{{ job.claimAuthor }}</strong>: {{ job.claimReason }}</li>
1919
</ul>
20-
<ul ng-if="job.shouldIndicateCulprits &amp;&amp; settings.showCulprits" class="culprits">
21-
<li data-ng-repeat="name in job.culprits">{{name}}</li>
20+
21+
<ul class="description">
22+
<li>{{ job.headline }}</li>
2223
</ul>
24+
2325
<ul ng-if="job.hasKnownFailures" class="failures">
2426
<ng-pluralize
2527
count="job.knownFailures.length"

‎src/main/resources/com/smartcodeltd/jenkinsci/plugins/buildmonitor/BuildMonitorView/main-settings.jelly

-7
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,6 @@
1919
id="settings-colour-blind" type="checkbox" />
2020
<label for="settings-colour-blind" title="Applies a colour blind-friendly colour scheme">Colour blind mode?</label>
2121
</li>
22-
<li>
23-
<input data-ng-model="settings.showCulprits"
24-
data-ng-false-value="0"
25-
data-ng-true-value="1"
26-
id="settings-show-culprits" type="checkbox" />
27-
<label for="settings-show-culprits" title="Shows authors who either broke or committed on a broken/unstable build">Show possible culprits?</label>
28-
</li>
2922
<li>
3023
<a class="btn"
3124
href="configure"

‎src/main/webapp/scripts/settings.js

-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ angular.
88
$scope.settings.fontSize = cookieJar.get('fontSize', 1);
99
$scope.settings.numberOfColumns = cookieJar.get('numberOfColumns', 2);
1010
$scope.settings.colourBlind = cookieJar.get('colourBlind', 0);
11-
$scope.settings.showCulprits = cookieJar.get('showCulprits', 1);
1211

1312
angular.forEach($scope.settings, function(value, name) {
1413
$scope.$watch('settings.' + name, function(currentValue) {

‎src/main/webapp/themes/industrial.css

+1-8
Original file line numberDiff line numberDiff line change
@@ -236,13 +236,6 @@ h2 {
236236
}
237237

238238
.build-monitor .meta li { display: inline-block; }
239-
.build-monitor .meta .culprits li { padding-right:0; margin-right:0.5em; }
240-
.build-monitor .meta .culprits li:after { content: ","; }
241-
.build-monitor .meta .culprits li:last-child:after { content: ""; }
242-
243-
.build-monitor ul.culprits li:first-child:before {
244-
content: "Possible Culprits: ";
245-
}
246239

247240
.build-monitor .meta .failures li { margin-right:0.5em; }
248241
.build-monitor .meta .failures li:after { content: ","; }
@@ -532,7 +525,7 @@ h2 {
532525
width:100%;
533526
}
534527
.build-monitor nav li:nth-child(1),
535-
.build-monitor nav li:nth-child(5) {
528+
.build-monitor nav li:nth-child(4) {
536529
margin-top:2em;
537530
}
538531
.build-monitor nav li:last-child {

‎src/test/java/com/smartcodeltd/jenkinsci/plugins/buildmonitor/viewmodel/JobViewTest.java

+25-60
Original file line numberDiff line numberDiff line change
@@ -13,22 +13,12 @@
1313
import static com.smartcodeltd.jenkinsci.plugins.buildmonitor.viewmodel.syntacticsugar.Sugar.*;
1414
import static com.smartcodeltd.jenkinsci.plugins.buildmonitor.viewmodel.syntacticsugar.TimeMachine.assumeThat;
1515
import static com.smartcodeltd.jenkinsci.plugins.buildmonitor.viewmodel.syntacticsugar.TimeMachine.currentTime;
16-
import static hudson.model.Result.ABORTED;
17-
import static hudson.model.Result.FAILURE;
18-
import static hudson.model.Result.SUCCESS;
19-
import static hudson.model.Result.UNSTABLE;
20-
import static org.hamcrest.CoreMatchers.hasItems;
21-
import static org.hamcrest.CoreMatchers.not;
16+
import static hudson.model.Result.*;
2217
import static org.hamcrest.Matchers.contains;
2318
import static org.hamcrest.Matchers.containsString;
24-
import static org.hamcrest.Matchers.hasItem;
25-
import static org.hamcrest.collection.IsCollectionWithSize.hasSize;
2619
import static org.hamcrest.core.Is.is;
2720
import static org.junit.Assert.assertThat;
28-
import static org.mockito.Mockito.mock;
29-
import static org.mockito.Mockito.times;
30-
import static org.mockito.Mockito.verify;
31-
import static org.mockito.Mockito.when;
21+
import static org.mockito.Mockito.*;
3222

3323
/**
3424
* @author Jan Molak
@@ -347,77 +337,53 @@ public void should_describe_the_job_as_claimed_if_someone_claimed_last_build_fai
347337
}
348338

349339
/*
350-
* Should know who broke the build
340+
* Executive summary
351341
*/
352342

353343
@Test
354-
public void should_know_who_broke_the_build() {
344+
public void should_tell_who_broke_the_build() throws Exception {
355345
view = a(jobView().of(
356-
a(job().whereTheLast(build().wasBrokenBy("Adam", "Ben")))));
357-
358-
assertThat(view.culprits(), hasSize(2));
359-
assertThat(view.culprits(), hasItems("Adam", "Ben"));
360-
}
361-
362-
@Test
363-
public void should_know_who_has_been_committing_over_broken_build() {
364-
view = a(jobView().of(
365-
a(job().whereTheLast(build().wasBrokenBy("Adam")).
366-
andThePrevious(build().wasBrokenBy("Ben", "Connor")).
367-
andThePrevious(build().wasBrokenBy("Daniel")).
368-
andThePrevious(build().succeededThanksTo("Errol")))));
369-
370-
assertThat(view.culprits(), hasSize(4));
371-
assertThat(view.culprits(), hasItems("Adam", "Ben", "Connor", "Daniel"));
372-
assertThat(view.culprits(), not(hasItem("Errol")));
373-
}
374-
375-
@Test
376-
public void should_only_mention_each_culprit_once() {
377-
view = a(jobView().of(
378-
a(job().whereTheLast(build().wasBrokenBy("Adam")).
379-
andThePrevious(build().wasBrokenBy("Adam", "Ben")).
380-
andThePrevious(build().wasBrokenBy("Ben", "Connor")))));
346+
a(job().whereTheLast(build().wasBrokenBy("Adam")))));
381347

382-
assertThat(view.culprits(), hasSize(3));
383-
assertThat(view.culprits(), hasItems("Adam", "Ben", "Connor"));
348+
assertThat(view.headline(), is("Failed after Adam committed their changes"));
384349
}
385350

386351
@Test
387-
public void should_not_mention_any_culprits_if_the_build_was_successful() {
352+
public void should_list_committers_who_broke_the_build() throws Exception {
388353
view = a(jobView().of(
389-
a(job().whereTheLast(build().succeededThanksTo("Adam")))));
354+
a(job().whereTheLast(build().wasBrokenBy("Adam", "Ben")))));
390355

391-
assertThat(view.culprits(), hasSize(0));
356+
assertThat(view.headline(), is("Failed after Ben and Adam committed their changes"));
392357
}
393358

394359
@Test
395-
public void should_not_mention_any_culprits_if_the_build_was_successful_and_is_still_running() {
360+
public void should_tell_who_broke_the_previous_build_if_the_current_one_is_still_running() throws Exception {
396361
view = a(jobView().of(
397-
a(job().whereTheLast(build().isStillBuilding())
398-
.andThePrevious(build().succeededThanksTo("Adam")))));
362+
a(job().whereTheLast(build().isStillBuilding()).
363+
andThePrevious(build().wasBrokenBy("Ben")))));
399364

400-
assertThat(view.culprits(), hasSize(0));
365+
assertThat(view.headline(), is("Failed after Ben committed their changes"));
401366
}
402367

403368
@Test
404-
public void should_indicate_culprits_if_the_build_is_failing_and_not_claimed() {
369+
public void should_tell_the_number_of_broken_builds_since_the_last_broken_build() throws Exception {
405370
view = a(jobView().of(
406-
a(job().whereTheLast(build().wasBrokenBy("Adam"))))
407-
.augmented(with(Claim.class)));
371+
a(job().whereTheLast(build().wasBrokenBy("Adam")).
372+
andThePrevious(build().wasBrokenBy("Ben", "Connor")).
373+
andThePrevious(build().wasBrokenBy("Daniel")).
374+
andThePrevious(build().succeededThanksTo("Errol")))));
408375

409-
assertThat(view.shouldIndicateCulprits(), is(true));
410-
assertThat(view.culprits(), hasSize(1));
376+
assertThat(view.headline(), is("2 builds have failed since Daniel committed their changes"));
411377
}
412378

413379
@Test
414-
public void should_not_indicate_any_culprits_if_the_build_was_failing_but_is_now_claimed() {
380+
public void should_tell_the_number_of_broken_builds_since_the_last_build_broken_by_multiple_committers() throws Exception {
415381
view = a(jobView().of(
416-
a(job().whereTheLast(build().wasBrokenBy("Adam").and().wasClaimedBy("Ben", "Helping out Adam"))))
417-
.augmented(with(Claim.class)));
382+
a(job().whereTheLast(build().wasBrokenBy("Adam")).
383+
andThePrevious(build().wasBrokenBy("Ben", "Connor")).
384+
andThePrevious(build().succeededThanksTo("Daniel")))));
418385

419-
assertThat(view.shouldIndicateCulprits(), is(false));
420-
assertThat(view.culprits(), hasSize(1));
386+
assertThat(view.headline(), is("1 build has failed since Ben and Connor committed their changes"));
421387
}
422388

423389
/*
@@ -457,11 +423,10 @@ public void public_api_should_return_reasonable_defaults_for_jobs_that_never_run
457423

458424
assertThat(view.lastBuildName(), is(""));
459425
assertThat(view.lastBuildUrl(), is(""));
426+
assertThat(view.headline(), is(""));
460427
assertThat(view.lastBuildDuration(), is(""));
461428
assertThat(view.estimatedDuration(), is(""));
462429
assertThat(view.progress(), is(0));
463-
assertThat(view.shouldIndicateCulprits(), is(false));
464-
assertThat(view.culprits(), hasSize(0));
465430
assertThat(view.status(), is("unknown"));
466431
assertThat(view.isClaimed(), is(false));
467432
assertThat(view.hasKnownFailures(), is(false));

0 commit comments

Comments
 (0)