Repository: mockito/mockito
Issue: Release notes group improvements by labels
Labels: continuous integration

For clearer release notes overlook we should group improvements by labels. 

This was originally contributed by @szpak, then lost during the early days of continuous integration, now it's back and it will rock!!!


Issue comments:

Pull request review comments:
I'd rename this to `Incompatible changes with v1.x`

Good idea. Thanks!


Patch that resolved the issue:
diff --git a/buildSrc/src/main/groovy/org/mockito/release/gradle/notes/ReleaseNotesExtension.groovy b/buildSrc/src/main/groovy/org/mockito/release/gradle/notes/ReleaseNotesExtension.groovy
index 305609be99..05a6c47aaf 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/gradle/notes/ReleaseNotesExtension.groovy
+++ b/buildSrc/src/main/groovy/org/mockito/release/gradle/notes/ReleaseNotesExtension.groovy
@@ -17,6 +17,9 @@ class ReleaseNotesExtension {
     private final File workDir;
     private final String version;
 
+    //TODO SF document the behavior
+    Map labels = new LinkedHashMap<String, String>()
+
     ReleaseNotesExtension(File workDir, String version) {
         this.workDir = workDir
         this.version = version
@@ -49,7 +52,7 @@ class ReleaseNotesExtension {
         def prev = "v" + getPreviousVersion()
         def current = "HEAD"
         LOG.lifecycle("Building notes for revisions: {} -> {}", prev, current)
-        def newContent = builder.buildNotes(version, prev, current)
+        def newContent = builder.buildNotes(version, prev, current, labels)
         newContent
     }
 
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/GitNotesBuilder.java b/buildSrc/src/main/groovy/org/mockito/release/notes/GitNotesBuilder.java
index 071e550e35..352c603cab 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/GitNotesBuilder.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/GitNotesBuilder.java
@@ -2,8 +2,8 @@
 
 import org.mockito.release.exec.Exec;
 import org.mockito.release.notes.improvements.ImprovementSet;
-import org.mockito.release.notes.improvements.ImprovementsProvider;
 import org.mockito.release.notes.improvements.Improvements;
+import org.mockito.release.notes.improvements.ImprovementsProvider;
 import org.mockito.release.notes.vcs.ContributionSet;
 import org.mockito.release.notes.vcs.ContributionsProvider;
 import org.mockito.release.notes.vcs.Vcs;
@@ -12,6 +12,7 @@
 
 import java.io.File;
 import java.util.Date;
+import java.util.Map;
 
 class GitNotesBuilder implements NotesBuilder {
 
@@ -29,14 +30,14 @@ class GitNotesBuilder implements NotesBuilder {
         this.authTokenEnvVar = authTokenEnvVar;
     }
 
-    public String buildNotes(String version, String fromRevision, String toRevision) {
+    public String buildNotes(String version, String fromRevision, String toRevision, Map<String, String> labels) {
         LOG.info("Getting release notes between {} and {}", fromRevision, toRevision);
 
         ContributionsProvider contributionsProvider = Vcs.getGitProvider(Exec.getProcessRunner(workDir));
         ContributionSet contributions = contributionsProvider.getContributionsBetween(fromRevision, toRevision);
 
         ImprovementsProvider improvementsProvider = Improvements.getGitHubProvider(authTokenEnvVar);
-        ImprovementSet improvements = improvementsProvider.getImprovements(contributions);
+        ImprovementSet improvements = improvementsProvider.getImprovements(contributions, labels);
 
         return new NotesPrinter().printNotes(version, new Date(), contributions, improvements);
     }
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/NotesBuilder.java b/buildSrc/src/main/groovy/org/mockito/release/notes/NotesBuilder.java
index 7c3cee6c4d..10da616846 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/NotesBuilder.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/NotesBuilder.java
@@ -1,5 +1,7 @@
 package org.mockito.release.notes;
 
+import java.util.Map;
+
 /**
  * Builds the release notes text
  */
@@ -11,6 +13,8 @@ public interface NotesBuilder {
      * @param version the version of the release we're building the notes
      * @param fromRevision valid git revision (can be tag name or HEAD)
      * @param toRevision valid git revision (can be tag name or HEAD)
+     * @param labels GitHub labels to caption mapping
+     *  TODO SF the labels better, currently they are coupled way too much with more generic interfaces, vcs agnostic
      */
-    String buildNotes(String version, String fromRevision, String toRevision);
+    String buildNotes(String version, String fromRevision, String toRevision, Map<String, String> labels);
 }
\ No newline at end of file
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/DefaultImprovements.java b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/DefaultImprovements.java
index bf7d55520a..dc605aeb68 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/DefaultImprovements.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/DefaultImprovements.java
@@ -1,29 +1,73 @@
 package org.mockito.release.notes.improvements;
 
-import java.util.LinkedList;
-import java.util.List;
+import org.mockito.release.util.MultiMap;
 
+import java.util.*;
+
+//TODO SF split the formatting from the data structure
 class DefaultImprovements implements ImprovementSet {
 
-    private final List<Improvement> improvements = new LinkedList<Improvement>();
+    final List<Improvement> improvements = new LinkedList<Improvement>();
+    private final Map<String, String> labels;
+
+    public DefaultImprovements(Map<String, String> labels) {
+        this.labels = labels;
+    }
 
     public String toText() {
         if (improvements.isEmpty()) {
-            //TODO SF we should break the build if there are no notable improvements yet the binaries have changed
             return "* No notable improvements. See the commits for detailed changes.";
         }
         StringBuilder sb = new StringBuilder("* Improvements: ").append(improvements.size());
-        for (Improvement i : improvements) {
-            sb.append("\n  * ").append(i.toText());
+        MultiMap<String, Improvement> byLabel = new MultiMap<String, Improvement>();
+        Set<Improvement> remainingImprovements = new LinkedHashSet<Improvement>(improvements);
+
+        //Step 1, find improvements that match input labels
+        //Iterate label first because the input labels determine the order
+        for (String label : labels.keySet()) {
+            for (Improvement i : improvements) {
+                if (i.labels.contains(label) && remainingImprovements.contains(i)) {
+                    remainingImprovements.remove(i);
+                    byLabel.put(label, i);
+                }
+            }
+        }
+
+        //Step 2, print out the improvements that match input labels
+        for (String label : byLabel.keySet()) {
+            String labelCaption = labels.get(label);
+            Collection<Improvement> labelImprovements = byLabel.get(label);
+            sb.append("\n  * ").append(labelCaption).append(": ").append(labelImprovements.size());
+            for (Improvement i : labelImprovements) {
+                sb.append("\n    * ").append(i.toText());
+            }
+        }
+
+        //Step 3, print out remaining changes
+        if (!remainingImprovements.isEmpty()) {
+            String indent;
+            //We want clean view depending if there are labelled improvements or not
+            if (byLabel.size() > 0) {
+                indent = "  ";
+                sb.append("\n  * Remaining changes: ").append(remainingImprovements.size());
+            } else {
+                indent = "";
+            }
+
+            for (Improvement i : remainingImprovements) {
+                sb.append("\n").append(indent).append("  * ").append(i.toText());
+            }
         }
         return sb.toString();
     }
 
-    public void add(Improvement improvement) {
+    public DefaultImprovements add(Improvement improvement) {
         improvements.add(improvement);
+        return this;
     }
 
-    public void addAll(List<Improvement> improvements) {
+    public DefaultImprovements addAll(List<Improvement> improvements) {
         this.improvements.addAll(improvements);
+        return this;
     }
 }
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubImprovementsProvider.java b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubImprovementsProvider.java
index c81e2c70ff..8258f6964a 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubImprovementsProvider.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubImprovementsProvider.java
@@ -4,6 +4,8 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.util.Map;
+
 class GitHubImprovementsProvider implements ImprovementsProvider {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(GitHubImprovementsProvider.class);
@@ -13,9 +15,9 @@ public GitHubImprovementsProvider(String authToken) {
         this.authToken = authToken;
     }
 
-    public ImprovementSet getImprovements(ContributionSet contributions) {
+    public ImprovementSet getImprovements(ContributionSet contributions, Map<String, String> labels) {
         LOGGER.info("Parsing {} commits with {} tickets", contributions.getAllCommits().size(), contributions.getAllTickets().size());
-        DefaultImprovements out = new DefaultImprovements();
+        DefaultImprovements out = new DefaultImprovements(labels);
         new GitHubTicketFetcher().fetchTickets(authToken, contributions.getAllTickets(), out);
         return out;
     }
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcher.java b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcher.java
index f3e3396f92..fcce09f99a 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcher.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcher.java
@@ -1,22 +1,18 @@
 package org.mockito.release.notes.improvements;
 
-import java.io.IOException;
-import java.io.InputStream;
-import java.net.URL;
-import java.net.URLConnection;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.List;
-import java.util.PriorityQueue;
-import java.util.Queue;
+import org.json.simple.JSONArray;
 import org.json.simple.JSONObject;
 import org.json.simple.JSONValue;
 import org.mockito.release.notes.util.IOUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+import java.net.URLConnection;
+import java.util.*;
+
 class GitHubTicketFetcher {
 
     private static final Logger LOG = LoggerFactory.getLogger(GitHubTicketFetcher.class);
@@ -70,6 +66,7 @@ private Queue<Long> queuedTicketNumbers(Collection<String> ticketIds) {
         return longs;
     }
 
+    //TODO SF we should be able to unit test the code that parsers JSONObjects
     private List<Improvement> wantedImprovements(Collection<Long> tickets, List<JSONObject> issues) {
         if(tickets.isEmpty()) {
             return Collections.emptyList();
@@ -81,7 +78,8 @@ private List<Improvement> wantedImprovements(Collection<Long> tickets, List<JSON
             if (tickets.remove(id)) {
                 String issueUrl = (String) issue.get("html_url");
                 String title = (String) issue.get("title");
-                pagedImprovements.add(new Improvement(id, title, issueUrl, new HashSet<String>()));
+                Collection<String> labels = extractLabels(issue);
+                pagedImprovements.add(new Improvement(id, title, issueUrl, labels));
 
                 if (tickets.isEmpty()) {
                     return pagedImprovements;
@@ -91,6 +89,15 @@ private List<Improvement> wantedImprovements(Collection<Long> tickets, List<JSON
         return pagedImprovements;
     }
 
+    private static Collection<String> extractLabels(JSONObject issue) {
+        Set<String> out = new HashSet<String>();
+        JSONArray labels = (JSONArray) issue.get("labels");
+        for (Object o : labels.toArray()) {
+            JSONObject label = (JSONObject) o;
+            out.add((String) label.get("name"));
+        }
+        return out;
+    }
 
     private static class GitHubIssues {
         public static final String RELATIVE_LINK_NOT_FOUND = "none";
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/Improvement.java b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/Improvement.java
index 4df57c3ad2..b5d7329cb5 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/Improvement.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/Improvement.java
@@ -4,10 +4,10 @@
 
 class Improvement {
 
-    private final long id;
+    private final long id; //TODO SF String
     private final String title;
     private final String url;
-    private final Collection<String> labels;
+    final Collection<String> labels;
 
     public Improvement(long id, String title, String url, Collection<String> labels) {
         this.id = id;
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/ImprovementsProvider.java b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/ImprovementsProvider.java
index ca459628a1..f6e0c73529 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/ImprovementsProvider.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/improvements/ImprovementsProvider.java
@@ -2,7 +2,9 @@
 
 import org.mockito.release.notes.vcs.ContributionSet;
 
+import java.util.Map;
+
 public interface ImprovementsProvider {
 
-    ImprovementSet getImprovements(ContributionSet contributions);
+    ImprovementSet getImprovements(ContributionSet contributions, Map<String, String> labels);
 }
diff --git a/buildSrc/src/main/groovy/org/mockito/release/util/MultiMap.java b/buildSrc/src/main/groovy/org/mockito/release/util/MultiMap.java
new file mode 100644
index 0000000000..59b54e3169
--- /dev/null
+++ b/buildSrc/src/main/groovy/org/mockito/release/util/MultiMap.java
@@ -0,0 +1,35 @@
+package org.mockito.release.util;
+
+import java.util.*;
+
+/**
+ * Basic multi-map that contains multiple values per key
+ */
+public class MultiMap<K, V> {
+
+    private final Map<K, Collection<V>> data = new LinkedHashMap<K, Collection<V>>();
+
+    /**
+     * If the key does not exist, null is returned
+     */
+    public Collection<V> get(K key) {
+        return data.get(key);
+    }
+
+    public void put(K key, V value) {
+        Collection<V> elements = get(key);
+        if (elements == null) {
+            elements = new LinkedHashSet<V>();
+        }
+        elements.add(value);
+        data.put(key, elements);
+    }
+
+    public Set<K> keySet() {
+        return data.keySet();
+    }
+
+    public int size() {
+        return data.size();
+    }
+}
diff --git a/gradle/release.gradle b/gradle/release.gradle
index 5b0c5e9701..c2913179f0 100644
--- a/gradle/release.gradle
+++ b/gradle/release.gradle
@@ -25,7 +25,19 @@ apply plugin: 'release-notes'
 apply plugin: 'release'
 
 notes {
-    notesFile = project.file("doc/release-notes/official.md")
+    notesFile = file("doc/release-notes/official.md")
+    //label captions, in order of importance - this is the order they will appear in the release notes
+    labels = [
+        '1.* incompatible': 'Incompatible changes with previous major version (v1.x)',
+        'java-9': "Java 9 support",
+        'java-8': "Java 8 support",
+        'new feature': "New features",
+        'BDD': 'Behavior-Driven Development support',
+        'bug': "Bugfixes",
+        'enhancement': "Enhancements",
+        'android': "Android support",
+        'docs': 'Documentation'
+    ]
 }
 
 def dryRun = project.hasProperty('dryRun')


Test patch:
diff --git a/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/DefaultImprovementsTest.groovy b/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/DefaultImprovementsTest.groovy
index 5557bb3610..410a96cfe0 100644
--- a/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/DefaultImprovementsTest.groovy
+++ b/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/DefaultImprovementsTest.groovy
@@ -1,28 +1,69 @@
 package org.mockito.release.notes.improvements
 
 import spock.lang.Specification
-import spock.lang.Subject
 
 class DefaultImprovementsTest extends Specification {
 
-    @Subject improvements = new DefaultImprovements()
-
     def "empty improvements"() {
         expect:
-        improvements.toText() == "* No notable improvements. See the commits for detailed changes."
+        new DefaultImprovements([:]).toText() == "* No notable improvements. See the commits for detailed changes."
     }
 
-    def "with improvements"() {
-        improvements.add(new Improvement(100, "Fix bug x", "http://issues/100", ["bug"]))
-        improvements.add(new Improvement(122, "Javadoc update", "http://url/122", []))
-        improvements.add(new Improvement(125, "Some enh", "http://issues/125", ["enhancement", "custom"]))
-        improvements.add(new Improvement(126, "Some other enh", "http://issues/126", ["enhancement"]))
+    def "set of improvements in order"() {
+        def is = new DefaultImprovements([bug: "Bugfixes", enhancement: "Enhancements"])
+            .add(new Improvement(100, "Fix bug x", "http://issues/100", ["bug"]))
+            .add(new Improvement(122, "Javadoc update", "http://url/122", []))
+            .add(new Improvement(125, "Some enh", "http://issues/125", ["java-8", "enhancement", "bug"]))
+            .add(new Improvement(126, "Some other enh", "http://issues/126", ["enhancement"]))
+            .add(new Improvement(130, "Refactoring", "http://issues/130", ["java-8", "refactoring"]))
 
         expect:
-        improvements.toText() == """* Improvements: 4
-  * Fix bug x [(#100)](http://issues/100)
-  * Javadoc update [(#122)](http://url/122)
-  * Some enh [(#125)](http://issues/125)
-  * Some other enh [(#126)](http://issues/126)"""
+        is.toText() == """* Improvements: 5
+  * Bugfixes: 2
+    * Fix bug x [(#100)](http://issues/100)
+    * Some enh [(#125)](http://issues/125)
+  * Enhancements: 1
+    * Some other enh [(#126)](http://issues/126)
+  * Remaining changes: 2
+    * Javadoc update [(#122)](http://url/122)
+    * Refactoring [(#130)](http://issues/130)"""
+    }
+
+    def "no matching labels"() {
+        given:
+        def is = new DefaultImprovements([bug: "Bugfixes"])
+            .add(new Improvement(10, "Issue 10", "10", []))
+
+        expect: "the formatting is simplified"
+        is.toText() == """* Improvements: 1
+  * Issue 10 [(#10)](10)"""
+    }
+
+    def "no duplicated improvements"() {
+        given:
+        def is = new DefaultImprovements([bug: "Bugfixes", refactoring: "Refactorings"])
+            .add(new Improvement(10, "Issue 10", "10", ["bug", "refactoring"]))
+            .add(new Improvement(11, "Issue 11", "11", ["refactoring", "bug"]))
+
+        expect: "no duplication even though labels are overused"
+        is.toText() == """* Improvements: 2
+  * Bugfixes: 2
+    * Issue 10 [(#10)](10)
+    * Issue 11 [(#11)](11)"""
+    }
+
+    def "the order of labels is determined"() {
+        given:
+        //input label captions determine the order of labels:
+        def labels = [p0: "Priority 0", p1: "Priority 1"]
+        def imp1 = new Improvement(10, "Issue 10", "10", ["p0"])
+        def imp2 = new Improvement(11, "Issue 11", "11", ["p1"])
+
+        when:
+        def improvements = new DefaultImprovements(labels).addAll([imp1, imp2]).toText()
+        def reordered = new DefaultImprovements(labels).addAll([imp2, imp1]).toText()
+
+        then: "The order of labels is determined"
+        improvements == reordered
     }
 }
diff --git a/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcherTest.groovy b/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcherTest.groovy
index 9021379993..3a41b136bb 100644
--- a/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcherTest.groovy
+++ b/buildSrc/src/test/groovy/org/mockito/release/notes/improvements/GitHubTicketFetcherTest.groovy
@@ -10,12 +10,15 @@ class GitHubTicketFetcherTest extends Specification {
 
     @Subject fetcher = new GitHubTicketFetcher()
 
-    //TODO SF tidy up this and the test subject
+    //This is an integration test
+    //It's not ideal but it gives us a good smoke test
+    //So far it is not problematic to maintain :)
     def "fetches improvements from GitHub"() {
-        def impr = new DefaultImprovements()
+        def impr = new DefaultImprovements([:])
         def readOnlyToken = "a0a4c0f41c200f7c653323014d6a72a127764e17"
         when: fetcher.fetchTickets(readOnlyToken, ['109', '108', '99999', '112'], impr)
         then:
+        impr.improvements.get(0).labels == ["enhancement"] as Set
         impr.toText() == """* Improvements: 2
   * Allow instances of other classes in AdditionalAnswers.delegatesTo [(#112)](https://github.com/mockito/mockito/issues/112)
   * Clarify Spy vs Mock CALLS_REAL_METHODS [(#108)](https://github.com/mockito/mockito/issues/108)"""
diff --git a/buildSrc/src/test/groovy/org/mockito/release/util/MultiMapTest.groovy b/buildSrc/src/test/groovy/org/mockito/release/util/MultiMapTest.groovy
new file mode 100644
index 0000000000..5a7b03fe48
--- /dev/null
+++ b/buildSrc/src/test/groovy/org/mockito/release/util/MultiMapTest.groovy
@@ -0,0 +1,30 @@
+package org.mockito.release.util
+
+import spock.lang.Specification
+
+class MultiMapTest extends Specification {
+
+    def "empty multi map"() {
+        def map = new MultiMap<String, String>()
+
+        expect:
+        map.size() == 0
+        map.keySet().empty
+    }
+
+    def "contains multiple values per key"() {
+        def map = new MultiMap<String, String>();
+        map.put("1", "x")
+        map.put("1", "y")
+        map.put("2", "z")
+
+        expect:
+        map.get("1") == ["x", "y"] as Set
+        map.get("2") == ["z"] as Set
+        map.size() == 2
+        map.keySet() == ["1", "2"] as Set
+
+        //questionable but I decided to keep the impl simple:
+        map.get("3") == null
+    }
+}