Cleanup some GPU PyLint issues

Makes the following changes to //content/test/gpu:
1. Adds an explanation for why too-few-public-methods shouldn't be
   removed.
2. Removes the general broad-except disable and inline disables it
   in the few valid use cases we have.
3. Removes the general too-many-branches disable and inline disables or
   fixes the resulting errors.

Bug: 1076144
Change-Id: If357e34f3e935464db54136b87f7c260ba4e5e22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303597Reviewed-by: Yuly Novikov <ynovikov@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789680}

content/test/gpu/gpu_tests/gpu_helper.py

@@ -202,8 +202,9 @@ def MatchDriverTag(tag):
 
 
 # No good way to reduce the number of local variables, particularly since each
-# argument is also considered a local.
-# pylint: disable=too-many-locals
+# argument is also considered a local. Also no good way to reduce the number of
+# branches without harming readability.
+# pylint: disable=too-many-locals,too-many-branches
 def EvaluateVersionComparison(version,
                               operation,
                               ref_version,
@@ -273,7 +274,7 @@ def EvaluateVersionComparison(version,
     raise Exception('Invalid operation: ' + operation)
 
   return operation == 'eq' or operation == 'ge' or operation == 'le'
-# pylint: enable=too-many-locals
+# pylint: enable=too-many-locals,too-many-branches
 
 
 def ExpectationsDriverTags():

content/test/gpu/gpu_tests/gpu_integration_test.py

@@ -195,7 +195,7 @@ class GpuIntegrationTest(
         super(GpuIntegrationTest, cls).StartBrowser()
         cls.tab = cls.browser.tabs[0]
         return
-      except Exception:
+      except Exception:  # pylint: disable=broad-except
         logging.exception('Browser start failed (attempt %d of %d). Backtrace:',
                           x, _START_BROWSER_RETRIES)
         # If we are on the last try and there is an exception take a screenshot

content/test/gpu/gpu_tests/gpu_process_integration_test.py

@@ -296,32 +296,24 @@ class GpuProcessIntegrationTest(gpu_integration_test.GpuIntegrationTest):
     feature_status_list = self.tab.EvaluateJavaScript(
         'browserBridge.gpuInfo.featureStatus.featureStatus')
     for name, status in feature_status_list.items():
-      if name == 'webgl':
-        if status != 'unavailable_software':
-          self.fail('WebGL status for SwiftShader failed: %s' % status)
-          return
-      elif name == '2d_canvas':
-        if status != 'unavailable_software':
-          self.fail('2D Canvas status for SwiftShader failed: %s' % status)
-          return
-      else:
-        pass
-    if not sys.platform.startswith('linux'):
-      # On Linux we relaunch GPU process to fallback to SwiftShader, therefore
-      # featureStatusForHardwareGpu isn't available.
-      feature_status_for_hardware_gpu_list = self.tab.EvaluateJavaScript(
-          'browserBridge.gpuInfo.featureStatusForHardwareGpu.featureStatus')
-      for name, status in feature_status_for_hardware_gpu_list.items():
-        if name == 'webgl':
-          if status != 'unavailable_off':
-            self.fail('WebGL status for hardware GPU failed: %s' % status)
-            return
-        elif name == '2d_canvas':
-          if status != 'enabled':
-            self.fail('2D Canvas status for hardware GPU failed: %s' % status)
-            return
-        else:
-          pass
+      if name == 'webgl' and status != 'unavailable_software':
+        self.fail('WebGL status for SwiftShader failed: %s' % status)
+      elif name == '2d_canvas' and status != 'unavailable_software':
+        self.fail('2D Canvas status for SwiftShader failed: %s' % status)
+
+    # On Linux we relaunch GPU process to fallback to SwiftShader, therefore
+    # featureStatusForHardwareGpu isn't available. So finish early if we're on
+    # Linux.
+    if sys.platform.startswith('linux'):
+      return
+
+    feature_status_for_hardware_gpu_list = self.tab.EvaluateJavaScript(
+        'browserBridge.gpuInfo.featureStatusForHardwareGpu.featureStatus')
+    for name, status in feature_status_for_hardware_gpu_list.items():
+      if name == 'webgl' and status != 'unavailable_off':
+        self.fail('WebGL status for hardware GPU failed: %s' % status)
+      elif name == '2d_canvas' and status != 'enabled':
+        self.fail('2D Canvas status for hardware GPU failed: %s' % status)
 
   def _GpuProcess_one_extra_workaround(self, test_path):
     # Start this test by launching the browser with no command line

content/test/gpu/gpu_tests/skia_gold_integration_test_base.py

@@ -510,7 +510,7 @@ class SkiaGoldIntegrationTestBase(gpu_integration_test.GpuIntegrationTest):
       # related to Gold.
       try:
         self._UploadTestResultToSkiaGold(image_name, screenshot, page)
-      except Exception as gold_exception:
+      except Exception as gold_exception:  # pylint: disable=broad-except
         logging.error(str(gold_exception))
       # TODO(https://crbug.com/1043129): Switch this to just "raise" once these
       # tests are run with Python 3. Python 2's behavior with nested try/excepts

content/test/gpu/gpu_tests/test_expectations_unittest.py

@@ -114,7 +114,7 @@ def _MapGpuDevicesToVendors(tag_sets):
 
 # No good way to reduce the number of return statements to the required level
 # without harming readability.
-# pylint: disable=too-many-return-statements
+# pylint: disable=too-many-return-statements,too-many-branches
 def _IsDriverTagDuplicated(driver_tag1, driver_tag2):
   if driver_tag1 == driver_tag2:
     return True
@@ -160,7 +160,7 @@ def _IsDriverTagDuplicated(driver_tag1, driver_tag2):
     return not gpu_helper.EvaluateVersionComparison(version1, 'le', version2)
   else:
     assert False
-# pylint: enable=too-many-return-statements
+# pylint: enable=too-many-return-statements,too-many-branches
 
 
 def _DoTagsConflict(t1, t2):

content/test/gpu/gpu_tests/trace_integration_test.py

@@ -239,6 +239,13 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
     return self.GetOverlayBotConfig(os_version_name, gpu.vendor_id,
                                     gpu.device_id)
 
+  def _GetAndAssertOverlayBotConfig(self):
+    overlay_bot_config = self.GetOverlayBotConfig()
+    if overlay_bot_config is None:
+      self.fail('Overlay bot config can not be determined')
+    assert overlay_bot_config.get('direct_composition', False)
+    return overlay_bot_config
+
   @staticmethod
   def _SwapChainPresentationModeToStr(presentation_mode):
     if presentation_mode == _SWAP_CHAIN_PRESENTATION_MODE_COMPOSED:
@@ -281,29 +288,23 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
     else:
       self.fail('Trace markers for GPU category %s were not found' % category)
 
-  def _EvaluateSuccess_CheckVideoPath(self, category, event_iterator,
-                                      other_args):
-    """Verifies Chrome goes down the code path as expected.
+  def _GetVideoPathExpectations(self, other_args):
+    """Helper method to get expectations for CheckVideoPath.
 
-    Depending on whether hardware overlays are supported or not, which formats
-    are supported in overlays, whether video is downscaled or not, whether
-    video is rotated or not, Chrome's video presentation code path can be
-    different.
-    """
-    os_name = self.browser.platform.GetOSName()
-    assert os_name and os_name.lower() == 'win'
+    Args:
+      other_args: The |other_args| arg passed into the test.
 
-    # Calculate expectations.
-    other_args = other_args if other_args is not None else {}
+    Returns:
+      A _VideoExpectations instance with zero_copy, pixel_format, and no_overlay
+      filled in.
+    """
+    overlay_bot_config = self._GetAndAssertOverlayBotConfig()
     expect_yuy2 = other_args.get('expect_yuy2', False)
-    zero_copy = other_args.get('zero_copy', False)
+    expected = _VideoExpectations()
+    expected.zero_copy = other_args.get('zero_copy', False)
+    expected.pixel_format = "NV12"
+    expected.no_overlay = other_args.get('no_overlay', False)
 
-    overlay_bot_config = self.GetOverlayBotConfig()
-    if overlay_bot_config is None:
-      self.fail('Overlay bot config can not be determined')
-    assert overlay_bot_config.get('direct_composition', False)
-
-    expected_pixel_format = "NV12"
     supports_nv12_overlays = False
     if overlay_bot_config.get('supports_overlays', False):
       supports_yuy2_overlays = False
@@ -314,12 +315,27 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
       assert supports_yuy2_overlays or supports_nv12_overlays
       if expect_yuy2 or not supports_nv12_overlays:
         if overlay_bot_config['yuy2_overlay_support'] != 'SOFTWARE':
-          expected_pixel_format = "YUY2"
+          expected.pixel_format = "YUY2"
     if not supports_nv12_overlays or overlay_bot_config[
         'nv12_overlay_support'] == 'SOFTWARE':
-      zero_copy = False
+      expected.zero_copy = False
+
+    return expected
+
+  def _EvaluateSuccess_CheckVideoPath(self, category, event_iterator,
+                                      other_args):
+    """Verifies Chrome goes down the code path as expected.
 
-    expect_no_overlay = other_args.get('no_overlay', False)
+    Depending on whether hardware overlays are supported or not, which formats
+    are supported in overlays, whether video is downscaled or not, whether
+    video is rotated or not, Chrome's video presentation code path can be
+    different.
+    """
+    os_name = self.browser.platform.GetOSName()
+    assert os_name and os_name.lower() == 'win'
+
+    other_args = other_args or {}
+    expected = self._GetVideoPathExpectations(other_args)
 
     # Verify expectations through captured trace events.
     for event in event_iterator:
@@ -327,47 +343,57 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
         continue
       if event.name != _SWAP_CHAIN_PRESENT_EVENT_NAME:
         continue
-      if expect_no_overlay:
+      if expected.no_overlay:
         self.fail('Expected no overlay got %s' % _SWAP_CHAIN_PRESENT_EVENT_NAME)
       detected_pixel_format = event.args.get('PixelFormat', None)
       if detected_pixel_format is None:
         self.fail('PixelFormat is missing from event %s' %
                   _SWAP_CHAIN_PRESENT_EVENT_NAME)
-      if expected_pixel_format != detected_pixel_format:
+      if expected.pixel_format != detected_pixel_format:
         self.fail('SwapChain pixel format mismatch, expected %s got %s' %
-                  (expected_pixel_format, detected_pixel_format))
+                  (expected.pixel_format, detected_pixel_format))
       detected_zero_copy = event.args.get('ZeroCopy', None)
       if detected_zero_copy is None:
         self.fail('ZeroCopy is missing from event %s' %
                   _SWAP_CHAIN_PRESENT_EVENT_NAME)
-      if zero_copy != detected_zero_copy:
+      if expected.zero_copy != detected_zero_copy:
         self.fail('ZeroCopy mismatch, expected %s got %s' %
-                  (zero_copy, detected_zero_copy))
+                  (expected.zero_copy, detected_zero_copy))
       break
     else:
-      if expect_no_overlay:
+      if expected.no_overlay:
         return
       self.fail(
           'Events with name %s were not found' % _SWAP_CHAIN_PRESENT_EVENT_NAME)
 
+  def _GetOverlayModeExpectations(self, other_args):
+    """Helper method to get expectations for CheckOverlayMode.
+
+    Args:
+      other_args: The |other_args| arg passed into the test.
+
+    Returns:
+      A _VideoExpectations instance with presentation_mode and no_overlay filled
+      in.
+    """
+    overlay_bot_config = self._GetAndAssertOverlayBotConfig()
+    expected = _VideoExpectations()
+    expected.presentation_mode = _SWAP_CHAIN_PRESENTATION_MODE_COMPOSED
+    expected.no_overlay = other_args.get('no_overlay', False)
+
+    if overlay_bot_config.get('supports_overlays', False):
+      if overlay_bot_config['nv12_overlay_support'] != 'SOFTWARE':
+        expected.presentation_mode = _SWAP_CHAIN_PRESENTATION_MODE_OVERLAY
+    return expected
+
   def _EvaluateSuccess_CheckOverlayMode(self, category, event_iterator,
                                         other_args):
     """Verifies video frames are promoted to overlays when supported."""
     os_name = self.browser.platform.GetOSName()
     assert os_name and os_name.lower() == 'win'
 
-    overlay_bot_config = self.GetOverlayBotConfig()
-    if overlay_bot_config is None:
-      self.fail('Overlay bot config can not be determined')
-    assert overlay_bot_config.get('direct_composition', False)
-
-    expected_presentation_mode = _SWAP_CHAIN_PRESENTATION_MODE_COMPOSED
-    if overlay_bot_config.get('supports_overlays', False):
-      if overlay_bot_config['nv12_overlay_support'] != 'SOFTWARE':
-        expected_presentation_mode = _SWAP_CHAIN_PRESENTATION_MODE_OVERLAY
-
-    other_args = other_args if other_args is not None else {}
-    expect_no_overlay = other_args.get('no_overlay', False)
+    other_args = other_args or {}
+    expected = self._GetOverlayModeExpectations(other_args)
 
     presentation_mode_history = []
     for event in event_iterator:
@@ -375,7 +401,7 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
         continue
       if event.name != _GET_STATISTICS_EVENT_NAME:
         continue
-      if expect_no_overlay:
+      if expected.no_overlay:
         self.fail('Expected no overlay got %s' % _GET_STATISTICS_EVENT_NAME)
       detected_presentation_mode = event.args.get('CompositionMode', None)
       if detected_presentation_mode is None:
@@ -383,7 +409,7 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
                   _GET_STATISTICS_EVENT_NAME)
       presentation_mode_history.append(detected_presentation_mode)
 
-    if expect_no_overlay:
+    if expected.no_overlay:
       return
 
     valid_entry_found = False
@@ -393,12 +419,12 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
           or mode == _SWAP_CHAIN_GET_FRAME_STATISTICS_MEDIA_FAILED):
         # Be more tolerant to avoid test flakiness
         continue
-      if mode != expected_presentation_mode:
+      if mode != expected.presentation_mode:
         if index >= len(presentation_mode_history) // 2:
           # Be more tolerant for the first half frames in non-overlay mode.
           self.fail('SwapChain presentation mode mismatch, expected %s got %s' %
                     (TraceIntegrationTest._SwapChainPresentationModeToStr(
-                        expected_presentation_mode),
+                        expected.presentation_mode),
                      TraceIntegrationTest._SwapChainPresentationModeListToStr(
                          presentation_mode_history)))
       valid_entry_found = True
@@ -441,6 +467,16 @@ class TraceIntegrationTest(gpu_integration_test.GpuIntegrationTest):
     ]
 
 
+class _VideoExpectations(object):
+  """Struct-like object for passing around video test expectations."""
+
+  def __init__(self):
+    self.pixel_format = None
+    self.zero_copy = None
+    self.no_overlay = None
+    self.presentation_mode = None
+
+
 def load_tests(loader, tests, pattern):
   del loader, tests, pattern  # Unused.
   return gpu_integration_test.LoadAllTestsInModule(sys.modules[__name__])

content/test/gpu/pylintrc

@@ -2,8 +2,7 @@
 
 # Disable the message, report, category or checker with the given id(s).
 # TODO(crbug.com/1076144): Shrink this list to as small as possible.
-disable=broad-except,
-        fixme,
+disable=fixme,
         invalid-name,
         locally-disabled,
         locally-enabled,
@@ -11,7 +10,6 @@ disable=broad-except,
         star-args,
         too-few-public-methods,
         too-many-arguments,
-        too-many-branches,
         too-many-function-args,
         too-many-instance-attributes,
         too-many-public-methods,
@@ -44,6 +42,11 @@ disable=broad-except,
 # There are valid cases where we want to selectively enable/disable lint errors
 # in a particular file/scope, e.g. allowing protected access in unittests.
 
+# too-few-public-methods
+# This is supposedly to catch "functions disguised as classes", but we have
+# never had issues with that. We do, however, use a handful of struct-like
+# classes, which this complains about.
+
 
 [REPORTS]