Split out Depends-On computation in helper classes This splitting out simplifies the main logic a bit, but helps a lot with getting simpler tests. Change-Id: Icd74a29a97bb9ef6132c3748129f351fb2984efd

diff --git a/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java b/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java
index f9e23b7..22b2f56 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java

@@ -14,9 +14,6 @@
 
 package com.googlesource.gerrit.plugins.zuul;
 
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.Project;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.Response;
@@ -25,24 +22,19 @@
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
-import com.googlesource.gerrit.plugins.zuul.util.CommitMessageFetcher;
+import com.googlesource.gerrit.plugins.zuul.util.DependsOnFetcher;
 import com.googlesource.gerrit.plugins.zuul.util.NeededByFetcher;
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 
 @Singleton
 public class GetCrd implements RestReadView<RevisionResource> {
-  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
-  private final CommitMessageFetcher commitMessageFetcher;
+  private final DependsOnFetcher dependsOnFetcher;
   private final NeededByFetcher neededByFetcher;
 
   @Inject
-  GetCrd(CommitMessageFetcher commitMessageFetcher, NeededByFetcher neededByFetcher) {
-    this.commitMessageFetcher = commitMessageFetcher;
+  GetCrd(DependsOnFetcher dependsOnFetcher, NeededByFetcher neededByFetcher) {
+    this.dependsOnFetcher = dependsOnFetcher;
     this.neededByFetcher = neededByFetcher;
   }
 
@@ -51,22 +43,10 @@
       throws RepositoryNotFoundException, IOException, BadRequestException, AuthException,
           PermissionBackendException {
     CrdInfo out = new CrdInfo();
-    Change.Key thisId = rsrc.getChange().getKey();
 
-    // get depends on info
-    out.dependsOn = new ArrayList<>();
-    Project.NameKey p = rsrc.getChange().getProject();
-    String rev = rsrc.getPatchSet().commitId().getName();
-    String commitMsg = commitMessageFetcher.fetch(p, rev);
-    Pattern pattern = Pattern.compile("[Dd]epends-[Oo]n:? (I[0-9a-f]{8,40})", Pattern.DOTALL);
-    Matcher matcher = pattern.matcher(commitMsg);
-    while (matcher.find()) {
-      String otherId = matcher.group(1);
-      logger.atFinest().log("Change %s depends on change %s", thisId, otherId);
-      out.dependsOn.add(otherId);
-    }
+    out.dependsOn = dependsOnFetcher.fetchForRevision(rsrc);
 
-    out.neededBy = neededByFetcher.fetchForChangeKey(thisId);
+    out.neededBy = neededByFetcher.fetchForChangeKey(rsrc.getChange().getKey());
 
     out.cycle = false;
     for (String neededKey : out.neededBy) {

diff --git a/src/main/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnExtractor.java b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnExtractor.java
new file mode 100644
index 0000000..92521a5
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnExtractor.java

@@ -0,0 +1,33 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.zuul.util;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/** Extracts dependency information from texts. */
+public class DependsOnExtractor {
+  public List<String> extract(String commitMessage) {
+    List<String> dependsOn = new ArrayList<>();
+    Pattern pattern = Pattern.compile("[Dd]epends-[Oo]n:? (I[0-9a-f]{8,40})", Pattern.DOTALL);
+    Matcher matcher = pattern.matcher(commitMessage);
+    while (matcher.find()) {
+      String key = matcher.group(1);
+      dependsOn.add(key);
+    }
+    return dependsOn;
+  }
+}

diff --git a/src/main/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnFetcher.java b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnFetcher.java
new file mode 100644
index 0000000..ee66de0
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnFetcher.java

@@ -0,0 +1,42 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.zuul.util;
+
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.change.RevisionResource;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.util.List;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+
+/** Fetches the Depends-On part of cross repository dependencies. */
+public class DependsOnFetcher {
+  private final CommitMessageFetcher commitMessageFetcher;
+  private final DependsOnExtractor dependsOnExtractor;
+
+  @Inject
+  public DependsOnFetcher(
+      CommitMessageFetcher commitMessageFetcher, DependsOnExtractor dependsOnExtractor) {
+    this.commitMessageFetcher = commitMessageFetcher;
+    this.dependsOnExtractor = dependsOnExtractor;
+  }
+
+  public List<String> fetchForRevision(RevisionResource rsrc)
+      throws RepositoryNotFoundException, IOException {
+    Project.NameKey p = rsrc.getChange().getProject();
+    String rev = rsrc.getPatchSet().commitId().getName();
+    String commitMsg = commitMessageFetcher.fetch(p, rev);
+    return dependsOnExtractor.extract(commitMsg);
+  }
+}

diff --git a/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java b/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java
index bfa3cdb..d5a101e 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java

@@ -17,30 +17,23 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.entities.Project;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.server.change.RevisionResource;
-import com.googlesource.gerrit.plugins.zuul.util.CommitMessageFetcher;
+import com.googlesource.gerrit.plugins.zuul.util.DependsOnFetcher;
 import com.googlesource.gerrit.plugins.zuul.util.NeededByFetcher;
-import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.List;
-import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
 
 public class GetCrdTest {
-  private NeededByFetcher neededByFetcher;
-  private CommitMessageFetcher commitMessageFetcher;
   private RevisionResource rsrc;
+  private DependsOnFetcher dependsOnFetcher;
+  private NeededByFetcher neededByFetcher;
 
   @Test
   public void testNoDependencies() throws Exception {
-    String commitMessage = "subject";
-    configureMocks(commitMessage, new ArrayList<>());
+    configureMocks(new ArrayList<>(), new ArrayList<>());
 
     GetCrd getCrd = createGetCrd();
     Response<CrdInfo> response = getCrd.apply(rsrc);
@@ -54,8 +47,10 @@
 
   @Test
   public void testSingleDependsOn() throws Exception {
-    String commitMessage = "subject\nDepends-On: I00000000";
-    configureMocks(commitMessage, new ArrayList<>());
+    ArrayList<String> dependsOn = new ArrayList<>();
+    dependsOn.add("I00000000");
+
+    configureMocks(dependsOn, new ArrayList<>());
 
     GetCrd getCrd = createGetCrd();
     Response<CrdInfo> response = getCrd.apply(rsrc);
@@ -69,9 +64,12 @@
 
   @Test
   public void testMultipleDependsOn() throws Exception {
-    String commitMessage =
-        "subject\nDepends-On: I00000000\nDepends-On: I00000002\nDepends-On: I00000004";
-    configureMocks(commitMessage, new ArrayList<>());
+    ArrayList<String> dependsOn = new ArrayList<>();
+    dependsOn.add("I00000000");
+    dependsOn.add("I00000002");
+    dependsOn.add("I00000004");
+
+    configureMocks(dependsOn, new ArrayList<>());
 
     GetCrd getCrd = createGetCrd();
     Response<CrdInfo> response = getCrd.apply(rsrc);
@@ -85,10 +83,12 @@
 
   @Test
   public void testSingleNeededBy() throws Exception {
-    String commitMessage = "subject";
+    List<String> dependsOn = new ArrayList<>();
+
     List<String> neededBy = new ArrayList<>();
     neededBy.add("I00000001");
-    configureMocks(commitMessage, neededBy);
+
+    configureMocks(dependsOn, neededBy);
 
     GetCrd getCrd = createGetCrd();
     Response<CrdInfo> response = getCrd.apply(rsrc);
@@ -102,12 +102,14 @@
 
   @Test
   public void testMultipleNeededBy() throws Exception {
-    String commitMessage = "subject";
+    List<String> dependsOn = new ArrayList<>();
+
     List<String> neededBy = new ArrayList<>();
     neededBy.add("I00000001");
     neededBy.add("I00000003");
     neededBy.add("I00000005");
-    configureMocks(commitMessage, neededBy);
+
+    configureMocks(dependsOn, neededBy);
 
     GetCrd getCrd = createGetCrd();
     Response<CrdInfo> response = getCrd.apply(rsrc);
@@ -121,11 +123,15 @@
 
   @Test
   public void testMixed() throws Exception {
-    String commitMessage = "subject\nDepends-On: I00000002\nDepends-On: I00000004";
+    List<String> dependsOn = new ArrayList<>();
+    dependsOn.add("I00000002");
+    dependsOn.add("I00000004");
+
     List<String> neededBy = new ArrayList<>();
     neededBy.add("I00000001");
     neededBy.add("I00000003");
-    configureMocks(commitMessage, neededBy);
+
+    configureMocks(dependsOn, neededBy);
 
     GetCrd getCrd = createGetCrd();
     Response<CrdInfo> response = getCrd.apply(rsrc);
@@ -139,10 +145,13 @@
 
   @Test
   public void testSimpleCycle() throws Exception {
-    String commitMessage = "subject\nDepends-On: I00000001";
+    List<String> dependsOn = new ArrayList<>();
+    dependsOn.add("I00000001");
+
     List<String> neededBy = new ArrayList<>();
     neededBy.add("I00000001");
-    configureMocks(commitMessage, neededBy);
+
+    configureMocks(dependsOn, neededBy);
 
     GetCrd getCrd = createGetCrd();
     Response<CrdInfo> response = getCrd.apply(rsrc);
@@ -154,36 +163,21 @@
     assertThat(crdInfo.cycle).isTrue();
   }
 
-  public void configureMocks(String commitMessage, final List<String> neededBy) throws Exception {
-    String commitId = "0123456789012345678901234567890123456789";
-
-    Project.NameKey projectNameKey = Project.nameKey("projectFoo");
-
-    PatchSet patchSet = mock(PatchSet.class);
-    when(patchSet.commitId()).thenReturn(ObjectId.fromString(commitId));
-
+  public void configureMocks(final List<String> dependsOn, final List<String> neededBy)
+      throws Exception {
     Change.Key changeKey = Change.key("I0123456789");
-
-    Change change =
-        new Change(
-            changeKey,
-            Change.id(4711),
-            Account.id(23),
-            BranchNameKey.create(projectNameKey, "branchBar"),
-            new Timestamp(0));
-
+    Change change = new Change(changeKey, null, null, null, null);
     rsrc = mock(RevisionResource.class);
     when(rsrc.getChange()).thenReturn(change);
-    when(rsrc.getPatchSet()).thenReturn(patchSet);
 
-    commitMessageFetcher = mock(CommitMessageFetcher.class);
-    when(commitMessageFetcher.fetch(projectNameKey, commitId)).thenReturn(commitMessage);
+    dependsOnFetcher = mock(DependsOnFetcher.class);
+    when(dependsOnFetcher.fetchForRevision(rsrc)).thenReturn(dependsOn);
 
     neededByFetcher = mock(NeededByFetcher.class);
     when(neededByFetcher.fetchForChangeKey(changeKey)).thenReturn(neededBy);
   }
 
   private GetCrd createGetCrd() {
-    return new GetCrd(commitMessageFetcher, neededByFetcher);
+    return new GetCrd(dependsOnFetcher, neededByFetcher);
   }
 }

diff --git a/src/test/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnExtractorTest.java b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnExtractorTest.java
new file mode 100644
index 0000000..7e24229
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnExtractorTest.java

@@ -0,0 +1,82 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.zuul.util;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.entities.Change;
+import java.util.List;
+import org.junit.Test;
+
+public class DependsOnExtractorTest {
+  @Test
+  public void testExtractNoDependies() throws Exception {
+    DependsOnExtractor extractor = new DependsOnExtractor();
+
+    List<String> extracted = extractor.extract("foo");
+
+    assertThat(extracted).isEmpty();
+  }
+
+  @Test
+  public void testExtractSingleDependency() throws Exception {
+    DependsOnExtractor extractor = new DependsOnExtractor();
+    String commitMessage = "subject\n\nDepends-On: " + getChangeKey(1);
+
+    List<String> extracted = extractor.extract(commitMessage);
+
+    assertThat(extracted).containsExactly(getChangeKey(1));
+  }
+
+  @Test
+  public void testExtractMultipleDependencies() throws Exception {
+    DependsOnExtractor extractor = new DependsOnExtractor();
+    String commitMessage =
+        "subject\n\nDepends-On: "
+            + getChangeKey(1)
+            + "\nDepends-On: "
+            + getChangeKey(2)
+            + "\nDepends-On: "
+            + getChangeKey(3);
+
+    List<String> extracted = extractor.extract(commitMessage);
+
+    assertThat(extracted).containsExactly(getChangeKey(1), getChangeKey(2), getChangeKey(3));
+  }
+
+  @Test
+  public void testExtractLowerCase() throws Exception {
+    DependsOnExtractor extractor = new DependsOnExtractor();
+    String commitMessage = "subject\n\ndepends-on: " + getChangeKey(1);
+
+    List<String> extracted = extractor.extract(commitMessage);
+
+    assertThat(extracted).containsExactly(getChangeKey(1));
+  }
+
+  @Test
+  public void testExtractMixedCases() throws Exception {
+    DependsOnExtractor extractor = new DependsOnExtractor();
+    String commitMessage =
+        "subject\n\ndepends-On: " + getChangeKey(1) + "\nDepends-on: " + getChangeKey(2);
+
+    List<String> extracted = extractor.extract(commitMessage);
+
+    assertThat(extracted).containsExactly(getChangeKey(1), getChangeKey(2));
+  }
+
+  private String getChangeKey(int keyEnding) {
+    return Change.key("I0123456789abcdef0000000000000000000" + (10000 + keyEnding)).toString();
+  }
+}

diff --git a/src/test/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnFetcherTest.java b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnFetcherTest.java
new file mode 100644
index 0000000..3cc6652
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/DependsOnFetcherTest.java

@@ -0,0 +1,101 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.zuul.util;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.change.RevisionResource;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Test;
+
+public class DependsOnFetcherTest {
+  private CommitMessageFetcher commitMessageFetcher;
+  private DependsOnExtractor dependsOnExtractor;
+  private RevisionResource rsrc;
+
+  @Test
+  public void testExtractNoDependencies() throws Exception {
+    configureMocks(new ArrayList<>());
+
+    DependsOnFetcher fetcher = createFetcher();
+    List<String> dependsOn = fetcher.fetchForRevision(rsrc);
+
+    assertThat(dependsOn).isEmpty();
+  }
+
+  @Test
+  public void testExtractSingleDependency() throws Exception {
+    List<String> extracted = new ArrayList<>();
+    extracted.add("I00000001");
+    configureMocks(extracted);
+
+    DependsOnFetcher fetcher = createFetcher();
+    List<String> dependsOn = fetcher.fetchForRevision(rsrc);
+
+    assertThat(dependsOn).containsExactly("I00000001");
+  }
+
+  @Test
+  public void testExtractMultipleDependencies() throws Exception {
+    List<String> extracted = new ArrayList<>();
+    extracted.add("I00000001");
+    extracted.add("I00000002");
+    extracted.add("I00000003");
+    configureMocks(extracted);
+
+    DependsOnFetcher fetcher = createFetcher();
+    List<String> dependsOn = fetcher.fetchForRevision(rsrc);
+
+    assertThat(dependsOn).containsExactly("I00000001", "I00000002", "I00000003");
+  }
+
+  private void configureMocks(List<String> dependsOn)
+      throws RepositoryNotFoundException, IOException {
+    String commitId = "0123456789012345678901234567890123456789";
+
+    Project.NameKey projectNameKey = Project.nameKey("projectFoo");
+
+    PatchSet patchSet = mock(PatchSet.class);
+    when(patchSet.commitId()).thenReturn(ObjectId.fromString(commitId));
+
+    BranchNameKey branchNameKey = BranchNameKey.create(projectNameKey, "branchBar");
+
+    Change.Key changeKey = Change.key("I0123456789");
+    Change change = new Change(changeKey, null, null, branchNameKey, null);
+
+    rsrc = mock(RevisionResource.class);
+    when(rsrc.getChange()).thenReturn(change);
+    when(rsrc.getPatchSet()).thenReturn(patchSet);
+
+    commitMessageFetcher = mock(CommitMessageFetcher.class);
+    when(commitMessageFetcher.fetch(projectNameKey, commitId)).thenReturn("commitMsgFoo");
+
+    dependsOnExtractor = mock(DependsOnExtractor.class);
+    when(dependsOnExtractor.extract("commitMsgFoo")).thenReturn(dependsOn);
+  }
+
+  private DependsOnFetcher createFetcher() {
+    return new DependsOnFetcher(commitMessageFetcher, dependsOnExtractor);
+  }
+}