Fix tests

We needed to add some expected output for a phabricator command.

Along the way, make the error reporting more informative.

Author: tomrittervg

apis/phabricator.py

@@ -84,7 +84,10 @@ def chain_revisions(parent_rev, child_rev):
             cmd += " | %s call-conduit --conduit-uri=%s differential.revision.search --""" % (_arc(), self.url)
 
             ret = self.run(cmd, shell=True)
-            result = json.loads(ret.stdout.decode())
+            try:
+                result = json.loads(ret.stdout.decode())
+            except Exception:
+                raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode())
 
             if result['error']:
                 raise Exception("Got an error from phabricator when trying to search for %s" % (child_rev))
@@ -101,7 +104,10 @@ def chain_revisions(parent_rev, child_rev):
             cmd = "echo " + quote_echo_string("""{"transactions": [{"type":"parents.add", "value":["%s"]}], "objectIdentifier": "%s"}""" % (child_phid, parent_rev))
             cmd += " | %s call-conduit --conduit-uri=%s differential.revision.edit --""" % (_arc(), self.url)
             ret = self.run(cmd, shell=True)
-            result = json.loads(ret.stdout.decode())
+            try:
+                result = json.loads(ret.stdout.decode())
+            except Exception:
+                raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode())
             if result['error']:
                 raise Exception("Got an error from phabricator when trying chain revisions, parent: %s, child %s %s" % (parent_rev, child_rev, child_phid))
 
@@ -116,7 +122,10 @@ def associate_bug_id(phab_revision):
             cmd = "echo " + quote_echo_string("""{"transactions": [{"type":"bugzilla.bug-id", "value":"%s"}], "objectIdentifier": "%s"}""" % (bug_id, phab_revision))
             cmd += " | %s call-conduit --conduit-uri=%s differential.revision.edit --""" % (_arc(), self.url)
             ret = self.run(cmd, shell=True)
-            result = json.loads(ret.stdout.decode())
+            try:
+                result = json.loads(ret.stdout.decode())
+            except Exception:
+                raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode())
             if result['error']:
                 raise Exception("Got an error from phabricator when trying to set the bugzilla id for %s" % (phab_revision))
 
@@ -142,7 +151,10 @@ def set_reviewer(self, phab_revision, phab_username):
             cmd += " | %s call-conduit --conduit-uri=%s user.search --""" % (_arc(), self.url)
 
         ret = self.run(cmd, shell=True)
-        result = json.loads(ret.stdout.decode())
+        try:
+            result = json.loads(ret.stdout.decode())
+        except Exception:
+            raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode())
 
         if result['error']:
             raise Exception("Got an error from phabricator when trying to search for %s" % (phab_username))
@@ -158,7 +170,10 @@ def set_reviewer(self, phab_revision, phab_username):
         cmd = "echo " + quote_echo_string("""{"transactions": [{"type":"reviewers.set", "value":["%s"]}], "objectIdentifier": "%s"}""" % (phid, phab_revision))
         cmd += " | %s call-conduit --conduit-uri=%s differential.revision.edit --""" % (_arc(), self.url)
         ret = self.run(cmd, shell=True)
-        result = json.loads(ret.stdout.decode())
+        try:
+            result = json.loads(ret.stdout.decode())
+        except Exception:
+            raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode())
         if result['error']:
             raise Exception("Got an error from phabricator when trying to set reviewers to %s (%s) for %s: %s" % (phab_username, phid, phab_revision, result))
 
@@ -168,7 +183,10 @@ def abandon(self, phab_revision):
         cmd = "echo " + quote_echo_string("""{"transactions": [{"type":"abandon", "value":true}],"objectIdentifier": "%s"}""" % phab_revision)
         cmd += " | %s call-conduit --conduit-uri=%s differential.revision.edit --""" % (_arc(), self.url)
         ret = self.run(cmd, shell=True)
-        result = json.loads(ret.stdout.decode())
+        try:
+            result = json.loads(ret.stdout.decode())
+        except Exception:
+            raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode())
         if result['error']:
             if "You can not abandon this revision because it has already been closed." in result['errorMessage']:
                 self.logger.log("Strangely, the phabricator revision %s was already closed when we tried to abandon it. Oh well." % phab_revision, level=LogLevel.Warning)

tests/functionality_utilities.py

@@ -78,10 +78,11 @@ def default_phab_submit():
         ("hg diff --stat", lambda: " accessible/interfaces/ia2/moz.build |  6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n"),
         ("arc diff --verbatim", command_callbacks.get('phab_submit', default_phab_submit)),
         ("arcanist diff --verbatim", command_callbacks.get('phab_submit', default_phab_submit)),
-        (echo_str("echo {\"constraints\""), lambda: CONDUIT_USERNAME_SEARCH_OUTPUT),
+        (echo_str("echo {\"constraints\""), lambda: CONDUIT_USERNAME_SEARCH_OUTPUT),  # This is also used for differential.revision.search, that's alright
         (echo_str("echo {\"transactions\": [{\"type\":\"reviewers.set\""), lambda: CONDUIT_EDIT_OUTPUT),
         (echo_str("echo {\"transactions\": [{\"type\":\"abandon\""), command_callbacks.get('abandon', AssertFalse)),
         (echo_str("echo {\"transactions\": [{\"type\":\"bugzilla.bug-id\""), lambda: CONDUIT_EDIT_OUTPUT),
+        (echo_str("echo {\"transactions\": [{\"type\":\"parents.add\""), lambda: CONDUIT_EDIT_OUTPUT),
         ("git log -1 --oneline", lambda: "0481f1c (HEAD -> issue-115-add-revision-to-log, origin/issue-115-add-revision-to-log) Issue #115 - Add revision of updatebot to log output"),
         ("git clone https://example.invalid .", lambda: ""),
         ("git merge-base", lambda: "_current"),
@@ -162,6 +163,8 @@ def TRY_OUTPUT(revision, include_auto_line=True):
 -> https://phabricator-dev.allizom.org/D%s
 """
 
+# This is also used for differential.revision.search, that's alright, because it returns a PHID
+# even if it's not the right type of PHID.
 CONDUIT_USERNAME_SEARCH_OUTPUT = """
 {"error":null,"errorMessage":null,"response":{"data":[{"id":154,"type":"USER","phid":"PHID-USER-dd6rge2k2csia46r2wcw","fields":{"username":"tjr","realName":"Tom Ritter","roles":["verified","approved","activated"],"dateCreated":1519415695,"dateModified":1519416233,"policy":{"view":"public","edit":"no-one"}},"attachments":[]}],"maps":[],"query":{"queryKey":null},"cursor":{"limit":100,"after":null,"before":null,"order":null}}}
 """