Use a Gerrit SHA number as a unique identifier for each corresponding CL patchset.

GitHub Node ID isn't unique to a taskcluster run. Instead, I suspect node id changes when Taskcluster changes status (from running to complete); see this example https://screenshot.googleplex.com/38i7BVticvY. Gerrit SHA numbers are always present in the PR description, unless a CL is merged; if so, the SHA in the comment is `Latest`

As a result of this change, at most one comment is made per patchset.

Bug: 1027618
Change-Id: I8c6c2006831db65cfe257a65faa01f1d33d75437
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225295Reviewed-by: Luke Z <lpz@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Auto-Submit: Kyle Ju <kyleju@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775119}

third_party/blink/tools/blinkpy/w3c/export_notifier.py

@@ -65,7 +65,6 @@ class ExportNotifier(object):
             gerrit_sha = self.wpt_github.extract_metadata(
                 WPT_REVISION_FOOTER, pr.body)
             gerrit_dict[gerrit_id] = PRStatusInfo(
-                taskcluster_status['node_id'],
                 taskcluster_status['target_url'], gerrit_sha)
 
         self.process_failing_prs(gerrit_dict)
@@ -129,7 +128,7 @@ class ExportNotifier(object):
             existing_status = PRStatusInfo.from_gerrit_comment(
                 message['message'])
             if existing_status:
-                return existing_status.node_id == pr_status_info.node_id
+                return existing_status.gerrit_sha == pr_status_info.gerrit_sha
 
         return False
 
@@ -176,23 +175,17 @@ class ExportNotifier(object):
 
 
 class PRStatusInfo(object):
-    NODE_ID_TAG = 'Taskcluster Node ID: '
     LINK_TAG = 'Taskcluster Link: '
     CL_SHA_TAG = 'Gerrit CL SHA: '
     PATCHSET_TAG = 'Patchset Number: '
 
-    def __init__(self, node_id, link, gerrit_sha=None):
-        self._node_id = node_id
+    def __init__(self, link, gerrit_sha=None):
         self._link = link
         if gerrit_sha:
             self._gerrit_sha = gerrit_sha
         else:
             self._gerrit_sha = 'Latest'
 
-    @property
-    def node_id(self):
-        return self._node_id
-
     @property
     def link(self):
         return self._link
@@ -203,11 +196,8 @@ class PRStatusInfo(object):
 
     @staticmethod
     def from_gerrit_comment(comment):
-        tags = [
-            PRStatusInfo.NODE_ID_TAG, PRStatusInfo.LINK_TAG,
-            PRStatusInfo.CL_SHA_TAG
-        ]
-        values = ['', '', '']
+        tags = [PRStatusInfo.LINK_TAG, PRStatusInfo.CL_SHA_TAG]
+        values = ['', '']
 
         for line in comment.splitlines():
             for index, tag in enumerate(tags):
@@ -226,12 +216,10 @@ class PRStatusInfo(object):
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.')
-        node_id_line = ('\n\n{}{}').format(PRStatusInfo.NODE_ID_TAG,
-                                           self.node_id)
-        link_line = ('\n{}{}').format(PRStatusInfo.LINK_TAG, self.link)
+        link_line = ('\n\n{}{}').format(PRStatusInfo.LINK_TAG, self.link)
         sha_line = ('\n{}{}').format(PRStatusInfo.CL_SHA_TAG, self.gerrit_sha)
 
-        comment = status_line + node_id_line + link_line + sha_line
+        comment = status_line + link_line + sha_line
         if patchset is not None:
             comment += ('\n{}{}').format(PRStatusInfo.PATCHSET_TAG, patchset)
 

third_party/blink/tools/blinkpy/w3c/export_notifier_unittest.py

@@ -24,13 +24,11 @@ class ExportNotifierTest(LoggingTestCase):
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: foo\n'
             'Taskcluster Link: bar\n'
             'Gerrit CL SHA: num')
 
         actual = PRStatusInfo.from_gerrit_comment(gerrit_comment)
 
-        self.assertEqual(actual.node_id, 'foo')
         self.assertEqual(actual.link, 'bar')
         self.assertEqual(actual.gerrit_sha, 'num')
 
@@ -40,8 +38,7 @@ class ExportNotifierTest(LoggingTestCase):
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: \n'
-            'Taskcluster Link: bar\n'
+            'Taskcluster Link: \n'
             'Gerrit CL SHA: num')
 
         actual = PRStatusInfo.from_gerrit_comment(gerrit_comment)
@@ -56,13 +53,12 @@ class ExportNotifierTest(LoggingTestCase):
         self.assertIsNone(actual)
 
     def test_to_gerrit_comment(self):
-        pr_status_info = PRStatusInfo('foo', 'bar', 'num')
+        pr_status_info = PRStatusInfo('bar', 'num')
         expected = (
             'The exported PR for the current patch failed Taskcluster check(s) '
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: foo\n'
             'Taskcluster Link: bar\n'
             'Gerrit CL SHA: num')
 
@@ -71,13 +67,12 @@ class ExportNotifierTest(LoggingTestCase):
         self.assertEqual(expected, actual)
 
     def test_to_gerrit_comment_latest(self):
-        pr_status_info = PRStatusInfo('foo', 'bar', None)
+        pr_status_info = PRStatusInfo('bar', None)
         expected = (
             'The exported PR for the current patch failed Taskcluster check(s) '
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: foo\n'
             'Taskcluster Link: bar\n'
             'Gerrit CL SHA: Latest')
 
@@ -86,13 +81,12 @@ class ExportNotifierTest(LoggingTestCase):
         self.assertEqual(expected, actual)
 
     def test_to_gerrit_comment_with_patchset(self):
-        pr_status_info = PRStatusInfo('foo', 'bar', 'num')
+        pr_status_info = PRStatusInfo('bar', 'num')
         expected = (
             'The exported PR for the current patch failed Taskcluster check(s) '
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: foo\n'
             'Taskcluster Link: bar\n'
             'Gerrit CL SHA: num\n'
             'Patchset Number: 3')
@@ -130,7 +124,7 @@ class ExportNotifierTest(LoggingTestCase):
                 taskcluster_status, 123), None)
 
     def test_has_latest_taskcluster_status_commented_false(self):
-        pr_status_info = PRStatusInfo('foo', 'bar', 'num')
+        pr_status_info = PRStatusInfo('bar', 'num')
         messages = [{
             "date": "2019-08-20 17:42:05.000000000",
             "message": "Uploaded patch set 1.\nInitial upload",
@@ -143,7 +137,7 @@ class ExportNotifierTest(LoggingTestCase):
         self.assertFalse(actual)
 
     def test_has_latest_taskcluster_status_commented_true(self):
-        pr_status_info = PRStatusInfo('foo', 'bar', 'num')
+        pr_status_info = PRStatusInfo('bar', 'num')
         messages = [
             {
                 "date": "2019-08-20 17:42:05.000000000",
@@ -158,7 +152,6 @@ class ExportNotifierTest(LoggingTestCase):
                  'on GitHub, which could indict cross-broswer failures on the '
                  'exportable changes. Please contact ecosystem-infra@ team for '
                  'more information.\n\n'
-                 'Taskcluster Node ID: foo\n'
                  'Taskcluster Link: bar\n'
                  'Gerrit CL SHA: num\n'
                  'Patchset Number: 3'),
@@ -214,8 +207,7 @@ class ExportNotifierTest(LoggingTestCase):
                          'on GitHub, which could indict cross-broswer failures on the '
                          'exportable changes. Please contact ecosystem-infra@ team for '
                          'more information.\n\n'
-                         'Taskcluster Node ID: notfoo\n'
-                         'Taskcluster Link: bar\n'
+                         'Taskcluster Link: notbar\n'
                          'Gerrit CL SHA: notnum\n'
                          'Patchset Number: 3'),
                         "_revision_number":
@@ -229,13 +221,12 @@ class ExportNotifierTest(LoggingTestCase):
                 }
             },
             api=self.notifier.gerrit)
-        gerrit_dict = {'abc': PRStatusInfo('foo', 'bar', 'num')}
+        gerrit_dict = {'abc': PRStatusInfo('bar', 'num')}
         expected = (
             'The exported PR for the current patch failed Taskcluster check(s) '
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: foo\n'
             'Taskcluster Link: bar\n'
             'Gerrit CL SHA: num\n'
             'Patchset Number: 1')
@@ -269,9 +260,8 @@ class ExportNotifierTest(LoggingTestCase):
                          'on GitHub, which could indict cross-broswer failures on the '
                          'exportable changes. Please contact ecosystem-infra@ team for '
                          'more information.\n\n'
-                         'Taskcluster Node ID: foo\n'
                          'Taskcluster Link: bar\n'
-                         'Gerrit CL SHA: notnum\n'
+                         'Gerrit CL SHA: num\n'
                          'Patchset Number: 3'),
                         "_revision_number":
                         2
@@ -284,7 +274,7 @@ class ExportNotifierTest(LoggingTestCase):
                 }
             },
             api=self.notifier.gerrit)
-        gerrit_dict = {'abc': PRStatusInfo('foo', 'bar', 'num')}
+        gerrit_dict = {'abc': PRStatusInfo('bar', 'num')}
 
         self.notifier.process_failing_prs(gerrit_dict)
 
@@ -312,8 +302,7 @@ class ExportNotifierTest(LoggingTestCase):
                          'on GitHub, which could indict cross-broswer failures on the '
                          'exportable changes. Please contact ecosystem-infra@ team for '
                          'more information.\n\n'
-                         'Taskcluster Node ID: not foo\n'
-                         'Taskcluster Link: bar\n'
+                         'Taskcluster Link: notbar\n'
                          'Gerrit CL SHA: notnum\n'
                          'Patchset Number: 3'),
                         "_revision_number":
@@ -332,10 +321,9 @@ class ExportNotifierTest(LoggingTestCase):
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: foo\n'
             'Taskcluster Link: bar\n'
             'Gerrit CL SHA: Latest')
-        gerrit_dict = {'abc': PRStatusInfo('foo', 'bar', None)}
+        gerrit_dict = {'abc': PRStatusInfo('bar', None)}
 
         self.notifier.process_failing_prs(gerrit_dict)
 
@@ -348,7 +336,7 @@ class ExportNotifierTest(LoggingTestCase):
     def test_process_failing_prs_raise_gerrit_error(self):
         self.notifier.dry_run = False
         self.notifier.gerrit = MockGerritAPI(raise_error=True)
-        gerrit_dict = {'abc': PRStatusInfo('foo', 'bar', 'num')}
+        gerrit_dict = {'abc': PRStatusInfo('bar', 'num')}
 
         self.notifier.process_failing_prs(gerrit_dict)
 
@@ -398,8 +386,7 @@ class ExportNotifierTest(LoggingTestCase):
                          'on GitHub, which could indict cross-broswer failures on the '
                          'exportable changes. Please contact ecosystem-infra@ team for '
                          'more information.\n\n'
-                         'Taskcluster Node ID: notfoo\n'
-                         'Taskcluster Link: bar\n'
+                         'Taskcluster Link: notbar\n'
                          'Gerrit CL SHA: notnum\n'
                          'Patchset Number: 3'),
                         "_revision_number":
@@ -418,7 +405,6 @@ class ExportNotifierTest(LoggingTestCase):
             'on GitHub, which could indict cross-broswer failures on the '
             'exportable changes. Please contact ecosystem-infra@ team for '
             'more information.\n\n'
-            'Taskcluster Node ID: foo\n'
             'Taskcluster Link: bar\n'
             'Gerrit CL SHA: hash\n'
             'Patchset Number: 2')