Revisit and rephrase my TODOs in blinkpy

Change-Id: I0959c592ba6e9ec9021b665bac221625021d7df1
Reviewed-on: https://chromium-review.googlesource.com/c/1355132
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612420}

third_party/blink/tools/blinkpy/common/net/layout_test_results.py

@@ -160,5 +160,4 @@ class LayoutTestResults(object):
         return sorted(results, key=lambda r: r.test_name())
 
     def didnt_run_as_expected_results(self):
-        # TODO(qyearsley): Rename this method.
         return self._filter_tests(lambda r: not r.did_run_as_expected())

third_party/blink/tools/blinkpy/common/system/log_testing.py

@@ -34,9 +34,8 @@ import logging
 import unittest
 
 # pylint: disable=invalid-name
-# Camel-case names are used here to match the style of the TestCase methods.
-# TODO(qyearsley): Change these names to use lowercase-only, for consistency
-# with other unit test helper methods.
+# Camel-case names were used here to match the style of the TestCase
+# methods. It would also be alright to change these to lowercase.
 
 
 class TestLogStream(object):
@@ -163,9 +162,6 @@ class LogTesting(object):
         """Returns the current list of log messages."""
         return self._test_stream.messages
 
-    # FIXME: Add a clearMessages() method for cases where the caller
-    #        deliberately doesn't want to assert every message.
-
     def assertMessages(self, messages):
         """Asserts the current array of log messages, and clear its contents.
 
@@ -227,8 +223,9 @@ class LoggingTestCase(unittest.TestCase):
         """Return the current list of log messages."""
         return self._log.messages()
 
-    # FIXME: Add a clearMessages() method for cases where the caller
-    #        deliberately doesn't want to assert every message.
+    # Note: If there's a case where the caller deliberately doesn't
+    # want to assert every message, a clearMessages() method could
+    # be added here.
 
     # See the docstring for LogTesting.assertMessages() for an explanation
     # of why we clear the array of messages after asserting its contents.

third_party/blink/tools/blinkpy/style/checker.py

@@ -390,8 +390,6 @@ class CheckerDispatcher(object):
         #        for this special case.
         basename = os.path.basename(file_path)
         if basename == 'TestExpectations':
-            # TODO(qyearsley): Replace hard-coded "TestExpectations" with a
-            # list of known "TestExpectations" files. Maybe shared with Port.
             return False
         for skipped_file in _SKIPPED_FILES_WITHOUT_WARNING:
             if self._should_skip_file_path(file_path, skipped_file):

third_party/blink/tools/blinkpy/tool/commands/rebaseline.py

@@ -109,8 +109,8 @@ class AbstractRebaseliningCommand(Command):
 class ChangeSet(object):
     """A record of TestExpectation lines to remove.
 
-    TODO(qyearsley): Remove this class, track list of lines to remove directly
-    in an attribute of AbstractRebaseliningCommand.
+    Note: This class is probably more complicated than necessary; it is mainly
+    used to track the list of lines that we want to remove from TestExpectations.
     """
 
     def __init__(self, lines_to_remove=None):
@@ -332,7 +332,7 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
                 except ValueError:
                     _log.debug('"%s" is not a JSON object, ignoring', line)
             if not updated:
-                # TODO(qyearsley): This probably should be an error. See http://crbug.com/649412.
+                # TODO(crbug.com/649412): This could be made into an error.
                 _log.debug('Could not add file based off output "%s"', stdout)
         return change_set
 

third_party/blink/tools/blinkpy/tool/commands/rebaseline_cl.py

@@ -80,8 +80,7 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
 
         # The WPT manifest is required when iterating through tests
         # TestBaselineSet if there are any tests in web-platform-tests.
-        # TODO(qyearsley): Consider calling ensure_manifest in BlinkTool.
-        # See: crbug.com/698294
+        # TODO(crbug.com/698294): Consider calling ensure_manifest in BlinkTool.
         WPTManifest.ensure_manifest(tool)
 
         if not self.check_ok_to_run():

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

@@ -150,10 +150,6 @@ class TestImporter(object):
         # TODO(crbug.com/800570 robertma): Re-enable it once we fix the bug.
         # self._delete_orphaned_baselines()
 
-        # TODO(qyearsley): Consider running the imported tests with
-        # `run_web_tests.py --reset-results external/wpt` to get some baselines
-        # before the try jobs are started.
-
         _log.info('Updating TestExpectations for any removed or renamed tests.')
         self.update_all_test_expectations_files(self._list_deleted_tests(), self._list_renamed_tests())
 
@@ -339,9 +335,8 @@ class TestImporter(object):
             _log.info('Applying exportable commit locally:')
             _log.info(commit.url())
             _log.info('Subject: %s', commit.subject().strip())
-            # TODO(qyearsley): We probably don't need to know about
-            # corresponding PRs at all anymore, although this information
-            # could still be useful for reference.
+            # Log a note about the corresponding PR.
+            # This might not be necessary, and could potentially be removed.
             pull_request = self.wpt_github.pr_for_chromium_commit(commit)
             if pull_request:
                 _log.info('PR: %spull/%d', WPT_GH_URL, pull_request.number)
@@ -424,9 +419,10 @@ class TestImporter(object):
 
         baselines = self.fs.files_under(self.dest_path, file_filter=is_baseline_filter)
 
-        # TODO(qyearsley): Factor out the manifest path to a common location.
-        # TODO(qyearsley): Factor out the manifest reading from here and Port
-        # to WPTManifest.
+        # Note about possible refactoring:
+        #  - the manifest path could be factored out to a common location, and
+        #  - the logic for reading the manifest could be factored out from here
+        # and the Port class.
         manifest_path = self.finder.path_from_layout_tests('external', 'wpt', 'MANIFEST.json')
         manifest = WPTManifest(self.fs.read_text_file(manifest_path))
         wpt_urls = manifest.all_urls()

third_party/blink/tools/blinkpy/web_tests/controllers/layout_test_runner.py

@@ -302,7 +302,7 @@ class Worker(object):
         self._update_test_input(test_input)
         start = time.time()
 
-        # TODO(qyearsley): Re-add logging if it doesn't create too much load (crbug.com/673207).
+        # TODO(crbug.com/673207): Re-add logging if it doesn't make the logs too large.
         self._caller.post('started_test', test_input)
         result = single_test_runner.run_single_test(
             self._port, self._options, self._results_directory, self._name,

third_party/blink/tools/blinkpy/web_tests/controllers/layout_test_runner_unittest.py

@@ -78,21 +78,6 @@ class LockCheckingRunner(LayoutTestRunner):
         self._tester = tester
         self._should_have_http_lock = http_lock
 
-    def handle_finished_list(self, source, list_name, num_tests, elapsed_time):
-        # TODO(qyearsley): This is never called; it should be fixed or removed.
-        self._tester.fail('This is never called')
-        if not self._finished_list_called:
-            self._tester.assertEqual(list_name, 'locked_tests')
-            self._tester.assertTrue(self._remaining_locked_shards)
-            self._tester.assertTrue(self._has_http_lock is self._should_have_http_lock)
-
-        super(LockCheckingRunner, self).handle_finished_list(source, list_name, num_tests, elapsed_time)
-
-        if not self._finished_list_called:
-            self._tester.assertEqual(self._remaining_locked_shards, [])
-            self._tester.assertFalse(self._has_http_lock)
-            self._finished_list_called = True
-
 
 class LayoutTestRunnerTests(unittest.TestCase):
 

third_party/blink/tools/blinkpy/web_tests/port/base.py

@@ -962,8 +962,8 @@ class Port(object):
         Note: this will not work with skipped directories. See also the same
         issue with update_all_test_expectations_files in test_importer.py.
         """
-        # TODO(qyearsley): Extract parsing logic (reading the file,
-        # constructing a parser, etc.) from here and test_copier.py.
+        # Note: The parsing logic here (reading the file, constructing a
+        # parser, etc.) is very similar to blinkpy/w3c/test_copier.py.
         path = self.path_to_never_fix_tests_file()
         contents = self._filesystem.read_text_file(path)
         parser = TestExpectationParser(self, all_tests=(), is_lint_mode=False)