From c9ca27b95710358fed14ae5665c2c618d4e5d4e1 Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sat, 27 Jun 2015 19:22:55 -0700 Subject: [PATCH 1/6] Break Differential up to make it testable Previously was mixing concerns with both talking to arcanist and presenting results. By breaking out a DifferentialClient that I can easily mock, we can test the core functionality without a ton of boilerplate. --- phabricator-plugin.iml | 6 +- pom.xml | 9 ++ .../phabricator/PhabricatorBuildWrapper.java | 6 +- .../phabricator/PhabricatorNotifier.java | 12 +- .../phabricator/conduit/Differential.java | 84 +----------- .../conduit/DifferentialClient.java | 122 ++++++++++++++++++ .../phabricator/conduit/DifferentialTest.java | 80 ++++++++++++ .../validDifferentialQueryResponse.json | 3 + 8 files changed, 235 insertions(+), 87 deletions(-) create mode 100644 src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java create mode 100644 src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java create mode 100644 src/test/resources/com/uber/jenkins/phabricator/conduit/validDifferentialQueryResponse.json diff --git a/phabricator-plugin.iml b/phabricator-plugin.iml index ba37b2f1..6f7ce38a 100644 --- a/phabricator-plugin.iml +++ b/phabricator-plugin.iml @@ -7,6 +7,7 @@ + @@ -121,6 +122,10 @@ + + + + @@ -259,7 +264,6 @@ - diff --git a/pom.xml b/pom.xml index 455dc026..fd243fc7 100644 --- a/pom.xml +++ b/pom.xml @@ -44,6 +44,7 @@ + org.apache.httpcomponents httpclient @@ -68,6 +69,14 @@ jna 3.2.2 + + + + test + org.mockito + mockito-core + 2.0.23-beta + diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java index 73c6c984..e10d1c80 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java @@ -24,6 +24,7 @@ import com.uber.jenkins.phabricator.conduit.ArcanistUsageException; import com.uber.jenkins.phabricator.conduit.Differential; import com.uber.jenkins.phabricator.utils.CommonUtils; +import com.uber.jenkins.phabricator.conduit.DifferentialClient; import hudson.EnvVars; import hudson.Launcher; import hudson.model.AbstractBuild; @@ -89,16 +90,17 @@ public Environment setUp(AbstractBuild build, } } + DifferentialClient diffClient = new DifferentialClient(diffID, starter, conduitToken, arcPath); Differential diff; try { - diff = new Differential(diffID, starter, conduitToken, arcPath); + diff = new Differential(diffClient); diff.decorate(build, this.getPhabricatorURL()); logger.println("Applying patch for differential"); // Post a silent notification if option is enabled if (showBuildStartedMessage) { - diff.postComment(diff.getBuildStartedMessage(environment)); + diffClient.postComment(diff.getBuildStartedMessage(environment)); } } catch (ArcanistUsageException e) { logger.println("[arcanist] unable to apply patch"); diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java index 190a76bd..b4d17c32 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java @@ -23,6 +23,7 @@ import com.uber.jenkins.phabricator.conduit.ArcanistUsageException; import com.uber.jenkins.phabricator.conduit.Differential; +import com.uber.jenkins.phabricator.conduit.DifferentialClient; import com.uber.jenkins.phabricator.uberalls.UberallsClient; import com.uber.jenkins.phabricator.utils.CommonUtils; import com.uber.jenkins.phabricator.utils.Logger; @@ -117,15 +118,16 @@ public final boolean perform(final AbstractBuild build, final Launcher lau LauncherFactory starter = new LauncherFactory(launcher, environment, listener.getLogger(), build.getWorkspace()); + DifferentialClient diffClient = new DifferentialClient(diffID, starter, conduitToken, arcPath); Differential diff; try { - diff = new Differential(diffID, starter, conduitToken, arcPath); + diff = new Differential(diffClient); } catch (ArcanistUsageException e) { logger.info("arcanist", "unable to fetch differential"); return true; } - String revisionID = diff.getRevisionID(); + String revisionID = diff.getRevisionID(true); if (CommonUtils.isBlank(revisionID)) { return this.ignoreBuild(logger.getStream(), "Unable to load revisionID from conduit for diff ID " + diffID); } @@ -172,7 +174,7 @@ public final boolean perform(final AbstractBuild build, final Launcher lau if (runHarbormaster) { logger.info("uberalls", "Sending build result to Harbormaster with PHID '" + phid + "', success: " + harbormasterSuccess); try { - diff.harbormaster(phid, harbormasterSuccess); + diffClient.harbormaster(phid, harbormasterSuccess); } catch (ArcanistUsageException e) { logger.info("arcanist", "unable to post to harbormaster"); return true; @@ -214,7 +216,7 @@ public final boolean perform(final AbstractBuild build, final Launcher lau JSONObject result = null; try { - result = diff.postComment(comment, silent, commentAction); + result = diffClient.postComment(comment, silent, commentAction); } catch (ArcanistUsageException e) { logger.info("arcanist", "unable to post comment"); } @@ -222,7 +224,7 @@ public final boolean perform(final AbstractBuild build, final Launcher lau logger.info("arcanist", "Get error " + result.get("errorMessage") + " with action " + commentAction + "; trying again with action 'none'"); try { - diff.postComment(comment, silent, "none"); + diffClient.postComment(comment, silent, "none"); } catch (ArcanistUsageException e) { logger.info("arcanist", "unable to post comment"); } diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java index 00873923..9f823180 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java @@ -25,7 +25,6 @@ import com.uber.jenkins.phabricator.PhabricatorPostbuildSummaryAction; import hudson.EnvVars; import hudson.model.AbstractBuild; -import hudson.model.Result; import net.sf.json.JSONException; import net.sf.json.JSONNull; import net.sf.json.JSONObject; @@ -33,44 +32,15 @@ import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; -import java.util.HashMap; -import java.util.Map; public class Differential { - private final JSONObject rawJSON; - private final LauncherFactory launcher; - private final String conduitToken; - private final String arcPath; + private JSONObject rawJSON; - public Differential(String diffID, LauncherFactory launcher, String conduitToken, String arcPath) throws IOException, InterruptedException, ArcanistUsageException { - this.conduitToken = conduitToken; - this.arcPath = arcPath; - this.launcher = launcher; - Map params = new HashMap(); - params.put("ids", new String[]{diffID}); + private final DifferentialClient client; - JSONObject query = this.callConduit("differential.querydiffs", params); - JSONObject response; - try { - response = query.getJSONObject("response"); - } catch (JSONException e) { - e.printStackTrace(); - throw new ArcanistUsageException( - String.format("No 'response' object found in conduit call: (%s) %s", - e.getMessage(), - query.toString(2))); - } - try { - this.rawJSON = response.getJSONObject(diffID); - } catch (JSONException e) { - e.printStackTrace(); - throw new ArcanistUsageException( - String.format("Unable to find '%s' key in response: (%s) %s", - diffID, - e.getMessage(), - response.toString(2))); - - } + public Differential(DifferentialClient client) throws ArcanistUsageException, IOException, InterruptedException { + this.client = client; + this.rawJSON = client.fetchDiff(); } public String getRevisionID(boolean formatted) { @@ -84,50 +54,6 @@ public String getRevisionID(boolean formatted) { return rawRevisionId; } - public String getRevisionID() { - return this.getRevisionID(true); - } - - /** - * Sets a harbormaster build status - * @param phid Phabricator object ID - * @param pass whether or not the build passed - * @throws IOException - * @throws InterruptedException - */ - public void harbormaster(String phid, boolean pass) throws IOException, InterruptedException, ArcanistUsageException { - Map params = new HashMap(); - params.put("type", pass ? "pass" : "fail"); - params.put("buildTargetPHID", phid); - - this.callConduit("harbormaster.sendmessage", params); - } - - private JSONObject callConduit(String methodName, Map params) throws IOException, InterruptedException, ArcanistUsageException { - ArcanistClient arc = new ArcanistClient(this.arcPath, "call-conduit", params, this.conduitToken, methodName); - return arc.parseConduit(this.launcher.launch(), this.launcher.getStderr()); - } - - /** - * Posts a comment to a differential - * @param message - * @param silent whether or not to trigger an email - * @param action phabricator comment action, e.g. 'resign', 'reject', 'none' - */ - public JSONObject postComment(String message, boolean silent, String action) throws IOException, InterruptedException, ArcanistUsageException { - Map params = new HashMap(); - params.put("revision_id", this.getRevisionID(false)); - params.put("action", action); - params.put("message", message); - params.put("silent", silent); - - return this.callConduit("differential.createcomment", params); - } - - public JSONObject postComment(String message) throws IOException, InterruptedException, ArcanistUsageException { - return postComment(message, true, "none"); - } - /** * Get the summary message * @param phabricatorURL diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java new file mode 100644 index 00000000..1f9a012d --- /dev/null +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java @@ -0,0 +1,122 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.jenkins.phabricator.conduit; + +import com.uber.jenkins.phabricator.LauncherFactory; +import net.sf.json.JSONException; +import net.sf.json.JSONObject; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +/** + * DifferentialClient handles all interaction with conduit/arc for differentials + */ +public class DifferentialClient { + private final String arcPath; + private final String conduitToken; + private final LauncherFactory launcher; + private final String diffID; + + public DifferentialClient(String diffID, LauncherFactory launcher, String arcPath, String conduitToken) { + this.diffID = diffID; + this.launcher = launcher; + this.arcPath = arcPath; + this.conduitToken = conduitToken; + } + + /** + * Posts a comment to a differential + * @param message + * @param silent whether or not to trigger an email + * @param action phabricator comment action, e.g. 'resign', 'reject', 'none' + */ + public JSONObject postComment(String message, boolean silent, String action) throws IOException, InterruptedException, ArcanistUsageException { + Map params = new HashMap(); + params.put("revision_id", this.diffID); + params.put("action", action); + params.put("message", message); + params.put("silent", silent); + + return this.callConduit("differential.createcomment", params); + } + + public JSONObject fetchDiff() throws ArcanistUsageException, IOException, InterruptedException { + Map params = new HashMap(); + params.put("ids", new String[]{this.diffID}); + JSONObject query = this.callConduit("differential.querydiffs", params); + JSONObject response; + try { + response = query.getJSONObject("response"); + } catch (JSONException e) { + e.printStackTrace(); + throw new ArcanistUsageException( + String.format("No 'response' object found in conduit call: (%s) %s", + e.getMessage(), + query.toString(2))); + } + try { + return response.getJSONObject(diffID); + } catch (JSONException e) { + e.printStackTrace(); + throw new ArcanistUsageException( + String.format("Unable to find '%s' key in response: (%s) %s", + diffID, + e.getMessage(), + response.toString(2))); + + } + } + + protected JSONObject callConduit(String methodName, Map params) throws IOException, InterruptedException, ArcanistUsageException { + ArcanistClient arc = new ArcanistClient(this.arcPath, "call-conduit", params, this.conduitToken, methodName); + return arc.parseConduit(this.launcher.launch(), this.launcher.getStderr()); + } + + /** + * Sets a harbormaster build status + * @param phid Phabricator object ID + * @param pass whether or not the build passed + * @throws IOException + * @throws InterruptedException + */ + public void harbormaster(String phid, boolean pass) throws IOException, InterruptedException, ArcanistUsageException { + Map params = new HashMap(); + params.put("type", pass ? "pass" : "fail"); + params.put("buildTargetPHID", phid); + + this.callConduit("harbormaster.sendmessage", params); + } + + + /** + * Post a comment on the differential + * @param message the string message to post + * @return + * @throws IOException + * @throws InterruptedException + * @throws ArcanistUsageException + */ + public JSONObject postComment(String message) throws IOException, InterruptedException, ArcanistUsageException { + return postComment(message, true, "none"); + } +} diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java new file mode 100644 index 00000000..b1175a9d --- /dev/null +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java @@ -0,0 +1,80 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.jenkins.phabricator.conduit; + + +import hudson.EnvVars; +import junit.framework.TestCase; +import net.sf.json.JSONObject; +import net.sf.json.groovy.JsonSlurper; +import org.junit.Rule; +import org.junit.Test; + +import static org.mockito.Mockito.*; + +import java.io.IOException; +import java.io.InputStream; + +public class DifferentialTest extends TestCase { + private static final String fakeDiffID = "not-a-real-id"; + @Rule + Differential d; + + public DifferentialTest() throws InterruptedException, ArcanistUsageException, IOException { + d = diffWithResponse(); + } + + @Test + public void testFetchRevisionID() throws Exception { + assertEquals(fakeDiffID, d.getRevisionID(false)); + } + + @Test + public void testGetPhabricatorLink() throws Exception { + assertTrue(d.getPhabricatorLink("http://example.com").contains(fakeDiffID)); + } + + @Test + public void testGetPhabricatorLinkInvalidURL() throws Exception { + // Try our best to join URLs, even when they are wrong + assertTrue(d.getPhabricatorLink("aoeu").contains("aoeu")); + } + + @Test + public void testGetBuildStartedMessage() throws Exception { + assertTrue(d.getBuildStartedMessage(new EnvVars()).contains("Build started")); + } + + private Differential diffWithResponse() throws InterruptedException, ArcanistUsageException, IOException { + DifferentialClient client = mock(DifferentialClient.class); + when(client.fetchDiff()).thenReturn(getValidQueryResponse()); + + return new Differential(client); + } + + private JSONObject getValidQueryResponse() throws IOException { + InputStream input = getClass().getResourceAsStream( + "validDifferentialQueryResponse.json" + ); + return (JSONObject) new JsonSlurper().parse(input); + } + +} diff --git a/src/test/resources/com/uber/jenkins/phabricator/conduit/validDifferentialQueryResponse.json b/src/test/resources/com/uber/jenkins/phabricator/conduit/validDifferentialQueryResponse.json new file mode 100644 index 00000000..55cb8da0 --- /dev/null +++ b/src/test/resources/com/uber/jenkins/phabricator/conduit/validDifferentialQueryResponse.json @@ -0,0 +1,3 @@ +{ + "revisionID": "not-a-real-id" +} \ No newline at end of file From de0d743a08806bc0512c88ffd3faa3c43da7895c Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sat, 27 Jun 2015 20:44:16 -0700 Subject: [PATCH 2/6] fix argument ordering --- .../uber/jenkins/phabricator/conduit/DifferentialClient.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java index 1f9a012d..3995181e 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java @@ -37,11 +37,11 @@ public class DifferentialClient { private final LauncherFactory launcher; private final String diffID; - public DifferentialClient(String diffID, LauncherFactory launcher, String arcPath, String conduitToken) { + public DifferentialClient(String diffID, LauncherFactory launcher, String conduitToken, String arcPath) { this.diffID = diffID; this.launcher = launcher; - this.arcPath = arcPath; this.conduitToken = conduitToken; + this.arcPath = arcPath; } /** From 69701e8b300db09694b77be1d55395345d67de6c Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sat, 27 Jun 2015 20:50:37 -0700 Subject: [PATCH 3/6] Also output HTML coverage reports for hoomans --- pom.xml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index fd243fc7..0cef59f3 100644 --- a/pom.xml +++ b/pom.xml @@ -92,7 +92,10 @@ cobertura-maven-plugin 2.6 - xml + + xml + html + 256m true From 1762407c198b11cfcf19262e3e918c00d11ce88b Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sun, 28 Jun 2015 22:20:29 -0700 Subject: [PATCH 4/6] address feedback --- .../phabricator/PhabricatorBuildWrapper.java | 2 +- .../phabricator/PhabricatorNotifier.java | 6 ++--- .../phabricator/conduit/Differential.java | 8 ++----- .../conduit/DifferentialClient.java | 19 +++++++-------- .../phabricator/conduit/DifferentialTest.java | 24 +++++++++---------- 5 files changed, 27 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java index e10d1c80..ebc6d6a9 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java @@ -93,7 +93,7 @@ public Environment setUp(AbstractBuild build, DifferentialClient diffClient = new DifferentialClient(diffID, starter, conduitToken, arcPath); Differential diff; try { - diff = new Differential(diffClient); + diff = new Differential(diffClient.fetchDiff()); diff.decorate(build, this.getPhabricatorURL()); logger.println("Applying patch for differential"); diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java index b4d17c32..388c0b91 100644 --- a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java +++ b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java @@ -121,7 +121,7 @@ public final boolean perform(final AbstractBuild build, final Launcher lau DifferentialClient diffClient = new DifferentialClient(diffID, starter, conduitToken, arcPath); Differential diff; try { - diff = new Differential(diffClient); + diff = new Differential(diffClient.fetchDiff()); } catch (ArcanistUsageException e) { logger.info("arcanist", "unable to fetch differential"); return true; @@ -174,9 +174,9 @@ public final boolean perform(final AbstractBuild build, final Launcher lau if (runHarbormaster) { logger.info("uberalls", "Sending build result to Harbormaster with PHID '" + phid + "', success: " + harbormasterSuccess); try { - diffClient.harbormaster(phid, harbormasterSuccess); + diffClient.sendHarbormasterMessage(phid, harbormasterSuccess); } catch (ArcanistUsageException e) { - logger.info("arcanist", "unable to post to harbormaster"); + logger.info("arcanist", "unable to post to sendHarbormasterMessage"); return true; } } else { diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java index 9f823180..3c9787de 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java @@ -20,12 +20,10 @@ package com.uber.jenkins.phabricator.conduit; -import com.uber.jenkins.phabricator.LauncherFactory; import com.uber.jenkins.phabricator.PhabricatorPostbuildAction; import com.uber.jenkins.phabricator.PhabricatorPostbuildSummaryAction; import hudson.EnvVars; import hudson.model.AbstractBuild; -import net.sf.json.JSONException; import net.sf.json.JSONNull; import net.sf.json.JSONObject; @@ -36,11 +34,9 @@ public class Differential { private JSONObject rawJSON; - private final DifferentialClient client; - public Differential(DifferentialClient client) throws ArcanistUsageException, IOException, InterruptedException { - this.client = client; - this.rawJSON = client.fetchDiff(); + public Differential(JSONObject rawJSON) throws ArcanistUsageException, IOException, InterruptedException { + this.rawJSON = rawJSON; } public String getRevisionID(boolean formatted) { diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java index 3995181e..270cfaee 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java @@ -68,7 +68,6 @@ public JSONObject fetchDiff() throws ArcanistUsageException, IOException, Interr try { response = query.getJSONObject("response"); } catch (JSONException e) { - e.printStackTrace(); throw new ArcanistUsageException( String.format("No 'response' object found in conduit call: (%s) %s", e.getMessage(), @@ -77,7 +76,6 @@ public JSONObject fetchDiff() throws ArcanistUsageException, IOException, Interr try { return response.getJSONObject(diffID); } catch (JSONException e) { - e.printStackTrace(); throw new ArcanistUsageException( String.format("Unable to find '%s' key in response: (%s) %s", diffID, @@ -87,24 +85,19 @@ public JSONObject fetchDiff() throws ArcanistUsageException, IOException, Interr } } - protected JSONObject callConduit(String methodName, Map params) throws IOException, InterruptedException, ArcanistUsageException { - ArcanistClient arc = new ArcanistClient(this.arcPath, "call-conduit", params, this.conduitToken, methodName); - return arc.parseConduit(this.launcher.launch(), this.launcher.getStderr()); - } - /** - * Sets a harbormaster build status + * Sets a sendHarbormasterMessage build status * @param phid Phabricator object ID * @param pass whether or not the build passed * @throws IOException * @throws InterruptedException */ - public void harbormaster(String phid, boolean pass) throws IOException, InterruptedException, ArcanistUsageException { + public JSONObject sendHarbormasterMessage(String phid, boolean pass) throws IOException, InterruptedException, ArcanistUsageException { Map params = new HashMap(); params.put("type", pass ? "pass" : "fail"); params.put("buildTargetPHID", phid); - this.callConduit("harbormaster.sendmessage", params); + return this.callConduit("sendHarbormasterMessage.sendmessage", params); } @@ -119,4 +112,10 @@ public void harbormaster(String phid, boolean pass) throws IOException, Interrup public JSONObject postComment(String message) throws IOException, InterruptedException, ArcanistUsageException { return postComment(message, true, "none"); } + + protected JSONObject callConduit(String methodName, Map params) throws IOException, InterruptedException, ArcanistUsageException { + ArcanistClient arc = new ArcanistClient(this.arcPath, "call-conduit", params, this.conduitToken, methodName); + return arc.parseConduit(this.launcher.launch(), this.launcher.getStderr()); + } + } diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java index b1175a9d..14104b7d 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java @@ -20,12 +20,11 @@ package com.uber.jenkins.phabricator.conduit; - import hudson.EnvVars; import junit.framework.TestCase; import net.sf.json.JSONObject; import net.sf.json.groovy.JsonSlurper; -import org.junit.Rule; +import org.junit.Before; import org.junit.Test; import static org.mockito.Mockito.*; @@ -34,40 +33,41 @@ import java.io.InputStream; public class DifferentialTest extends TestCase { - private static final String fakeDiffID = "not-a-real-id"; - @Rule - Differential d; + private static final String FAKE_DIFF_ID = "not-a-real-id"; + + Differential differential; - public DifferentialTest() throws InterruptedException, ArcanistUsageException, IOException { - d = diffWithResponse(); + @Before + public void setupDiff () throws InterruptedException, ArcanistUsageException, IOException { + differential = diffWithResponse(); } @Test public void testFetchRevisionID() throws Exception { - assertEquals(fakeDiffID, d.getRevisionID(false)); + assertEquals(FAKE_DIFF_ID, differential.getRevisionID(false)); } @Test public void testGetPhabricatorLink() throws Exception { - assertTrue(d.getPhabricatorLink("http://example.com").contains(fakeDiffID)); + assertTrue(differential.getPhabricatorLink("http://example.com").contains(FAKE_DIFF_ID)); } @Test public void testGetPhabricatorLinkInvalidURL() throws Exception { // Try our best to join URLs, even when they are wrong - assertTrue(d.getPhabricatorLink("aoeu").contains("aoeu")); + assertTrue(differential.getPhabricatorLink("aoeu").contains("aoeu")); } @Test public void testGetBuildStartedMessage() throws Exception { - assertTrue(d.getBuildStartedMessage(new EnvVars()).contains("Build started")); + assertTrue(differential.getBuildStartedMessage(new EnvVars()).contains("Build started")); } private Differential diffWithResponse() throws InterruptedException, ArcanistUsageException, IOException { DifferentialClient client = mock(DifferentialClient.class); when(client.fetchDiff()).thenReturn(getValidQueryResponse()); - return new Differential(client); + return new Differential(client.fetchDiff()); } private JSONObject getValidQueryResponse() throws IOException { From 2bf96fc0cfbab97ffb43482c699e0968c9e4740f Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sun, 28 Jun 2015 22:21:58 -0700 Subject: [PATCH 5/6] Address review feedback --- .../phabricator/conduit/DifferentialTest.java | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java index 14104b7d..664caf92 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java @@ -24,11 +24,8 @@ import junit.framework.TestCase; import net.sf.json.JSONObject; import net.sf.json.groovy.JsonSlurper; -import org.junit.Before; import org.junit.Test; -import static org.mockito.Mockito.*; - import java.io.IOException; import java.io.InputStream; @@ -37,9 +34,8 @@ public class DifferentialTest extends TestCase { Differential differential; - @Before - public void setupDiff () throws InterruptedException, ArcanistUsageException, IOException { - differential = diffWithResponse(); + protected void setUp() throws IOException, ArcanistUsageException, InterruptedException { + differential = new Differential(getValidQueryResponse()); } @Test @@ -63,13 +59,6 @@ public void testGetBuildStartedMessage() throws Exception { assertTrue(differential.getBuildStartedMessage(new EnvVars()).contains("Build started")); } - private Differential diffWithResponse() throws InterruptedException, ArcanistUsageException, IOException { - DifferentialClient client = mock(DifferentialClient.class); - when(client.fetchDiff()).thenReturn(getValidQueryResponse()); - - return new Differential(client.fetchDiff()); - } - private JSONObject getValidQueryResponse() throws IOException { InputStream input = getClass().getResourceAsStream( "validDifferentialQueryResponse.json" From 70aaad4039a201286b2ed0c5273e72e8ccd40431 Mon Sep 17 00:00:00 2001 From: Aiden Scandella Date: Sun, 28 Jun 2015 22:32:46 -0700 Subject: [PATCH 6/6] remove newline and stacktrace --- .../java/com/uber/jenkins/phabricator/conduit/Differential.java | 1 - .../com/uber/jenkins/phabricator/conduit/DifferentialTest.java | 1 - 2 files changed, 2 deletions(-) diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java index 3c9787de..021dd0b5 100644 --- a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java +++ b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java @@ -68,7 +68,6 @@ public String getPhabricatorLink(String phabricatorURL) { URL base = new URL(phabricatorURL); return new URL(base, revisionID).toString(); } catch (MalformedURLException e) { - e.printStackTrace(); return String.format("%s%s", phabricatorURL, revisionID); } } diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java index 664caf92..612e0e18 100644 --- a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java @@ -65,5 +65,4 @@ private JSONObject getValidQueryResponse() throws IOException { ); return (JSONObject) new JsonSlurper().parse(input); } - }