Pass through in dual_browser_story for disabled benchmarks.

tools/perf/page_sets/dual_browser_story.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import collections
 import logging
 import re
 import sys
 import urllib
 
@@ skipping 61 matching lines @@
     self._current_story = None
     self._current_browser = None
     self._current_tab = None
 
     possible_browser = self._PrepareBrowser('default', finder_options)
     if not possible_browser:
       raise browser_finder_exceptions.BrowserFinderException(
           'No browser found.\n\nAvailable browsers:\n%s\n' %
           '\n'.join(browser_finder.GetAllAvailableBrowserTypes(finder_options)))
     if not finder_options.run_disabled_tests:
       self._CheckTestEnabled(test, possible_browser)

[perezju, 2016/09/30 13:13:29]

I think now you can remove this check together with the method definition below;
no need for it anymore.

[eyaich1, 2016/09/30 13:22:35]

Why did we ever need it?  Would the execution ever get here on a disabled
benchmark given that it would go through the check in benchmark_runner and/or
story_runner?  Was this just a safety check?

I agree it is not needed though, removed.

[perezju, 2016/09/30 13:28:01]

I think I added it here _because_ of the other bug you fixed; probably because I
noticed that the benchmark was trying to run even when it was disabled.

 
     extra_browser_types = set(story.browser_type for story in story_set)
     extra_browser_types.remove('default')  # Must include 'default' browser.
     for browser_type in extra_browser_types:
       options = _OptionsForBrowser(browser_type, finder_options)
       if not self._PrepareBrowser(browser_type, options):
         logging.warning('Skipping %s (%s) because %s browser is not available',
                         test.__name__, str(test), browser_type)
         logging.warning('Install %s to be able to run the test.', browser_type)
         sys.exit(0)

[perezju, 2016/09/30 13:13:29]

Could you make this one die hard instead by raising an exception? Now it should
be an error to attempt to force-run this benchmark on a device without the
WebView shell installed.

[eyaich1, 2016/09/30 13:22:35]

Done.

 
     # TODO(crbug/404771): Move network controller options out of
     # browser_options and into finder_options.
     browser_options = finder_options.browser_options
     if finder_options.use_live_sites:
       wpr_mode = wpr_modes.WPR_OFF
     elif browser_options.wpr_mode == wpr_modes.WPR_RECORD:
       wpr_mode = wpr_modes.WPR_RECORD
     else:
       wpr_mode = wpr_modes.WPR_REPLAY
 
     self.platform.network_controller.Open(wpr_mode,
                                           browser_options.extra_wpr_args)
 
   @property
   def current_tab(self):
     return self._current_tab
 
   @property
   def platform(self):
     return self._platform
 
   def _CheckTestEnabled(self, test, possible_browser):
     should_skip, msg = decorators.ShouldSkip(test, possible_browser)
     if should_skip:
       logging.warning(msg)
       logging.warning('You are trying to run a disabled test.')
-      logging.warning(
-          'Pass --also-run-disabled-tests to squelch this message.')
-      sys.exit(0)
+      logging.warning('Valid chartjson ouput will not be generated.')

[perezju, 2016/09/29 13:01:45]

This will effectively cause the benchmark to run even if it's disabled, no?
Causing it to fail on devices where it cannot run.

Is there an alternative to make this produce a valid disabled chartjson?

[eyaich1, 2016/09/29 13:05:30]

Well it won't fail, it will still technically succeed it just wont upload
results and the json will return as results['enabled'] = False.  

As I mentioned in the team meeting yesterday, this is the only difference people
should see with this new logic and we may want to do something like print
DISABLED in the step or something since it will appear green even though it
didn't actually run.

I don't see an alternative to this as we made a conscious decision not to
dynamically generate the set of perf benchmarks to run but instead run them all
on each bot and just ignore the disabled ones.

[nednguyen, 2016/09/29 13:08:40]

Hmhh, why not define ShouldDisable(..) for the benchmarks that are using this
story?

 
   def _PrepareBrowser(self, browser_type, options):
     """Add a browser to the dict of possible browsers.
 
     TODO(perezju): When available, use the GetBrowserForPlatform API instead.
     See: crbug.com/570348
 
     Returns:
       The possible browser if found, or None otherwise.
     """
@@ skipping 129 matching lines @@
           url=GOOGLE_SEARCH + urllib.urlencode({'q': query}),
           browser_type='android-webview',
           phase='on_webview'))
 
       # Stories that run on the browser selected by command line options.
       self.AddStory(SinglePage(
           name=re.sub(r'\W+', '_', url),
           url=url,
           browser_type='default',
           phase='on_chrome'))