Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
820093002:
Integration tests for desktop platforms and safebrowsing (Closed)
|
Created:
6 years ago by Not at Google. Contact bengr Modified:
5 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntegration tests for desktop platforms and safebrowsing.
Fix timeout check: If _expect_timeout flag is set, and timeout does not occur, then raise an exception.
Add new test for safebrowsing for desktop: Confirm that safebrowsing is turned off on server when ChromeProxy header identifies request as originating from desktop platforms with c=<clienttype>.
Fix safebrowsing test for mobile: Remove checks for safebrowsing response headers since it is not visible to telemetry tests.
BUG=434769
Committed: https://crrev.com/58ee32e05d510fff9190c5f05fc9b0ea0df31ac1
Cr-Commit-Position: refs/heads/master@{#313098}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed code review comments. #
Total comments: 10
Patch Set 3 : Addressed code review comments #Patch Set 4 : Fixed merge #Patch Set 5 : whitespace. #
Total comments: 18
Patch Set 6 : Removed redundant variables. #
Total comments: 13
Patch Set 7 : Generic platform independent test names #
Total comments: 6
Patch Set 8 : Sync to head #Patch Set 9 : Changed tags #Patch Set 10 : Sync to head #
Total comments: 4
Patch Set 11 : Fixed test names. Add @classmethod. #
Messages
Total messages: 25 (2 generated)
kundaji@chromium.org changed reviewers: + bengr@chromium.org, bolian@chromium.org, bustamante@chromium.org, sclittle@chromium.org
https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:384: if page.name == 'safebrowsingmobile': You should remove this if statement if you're removing safebrowsing from the smoke test. https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py (right): https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py:64: # Page that should cause a bypass for iOS clients. Fix comment https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py:70: # Page that should cause a bypass for iOS clients. Fix comment
https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:384: if page.name == 'safebrowsingmobile': On 2014/12/29 18:51:35, sclittle wrote: > You should remove this if statement if you're removing safebrowsing from the > smoke test. Done. https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py (right): https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py:64: # Page that should cause a bypass for iOS clients. On 2014/12/29 18:51:35, sclittle wrote: > Fix comment Done. https://codereview.chromium.org/820093002/diff/1/tools/chrome_proxy/integrati... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py:70: # Page that should cause a bypass for iOS clients. On 2014/12/29 18:51:35, sclittle wrote: > Fix comment Done.
https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:163: self._expect_timeout = True The AddResultsForSafebrowsingDesktop method in metrics seems to expect all responses to come through the proxy, so why is a timeout expected? If you really expect a timeout, and for 0 responses to come in, then please change AddResultsForSafebrowsingDesktop to just expect 0 responses. https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:313: def AddResultsForSafebrowsingDesktop(self, tab, results): Move this method definition up above AddResultsForHTTPFallback. https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:316: for resp in self.IterResponses(tab): What do you actually expect to happen here? Should the DRP return a safebrowsing response, and the client should ignore it, or should the real response come in from the DRP, or should the DRP cause a bypass? Or should the client not send any request at all and pop up an interstitial? Please comment about what is expected to happen on desktop. https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:319: safebrowsing_count += 1 Why is this called safebrowsing_count if you're counting via headers? https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:328: results.current_page, 'safebrowsing', 'boolean', True)) Instead of just a boolean, how about adding both count and safebrowsing_count as results? You could update the regular safebrowsing test to report these as well.
https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:163: self._expect_timeout = True On 2015/01/22 23:15:14, sclittle wrote: > The AddResultsForSafebrowsingDesktop method in metrics seems to expect all > responses to come through the proxy, so why is a timeout expected? > > If you really expect a timeout, and for 0 responses to come in, then please > change AddResultsForSafebrowsingDesktop to just expect 0 responses. Fixed. No timeout expected here. https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:313: def AddResultsForSafebrowsingDesktop(self, tab, results): On 2015/01/22 23:15:14, sclittle wrote: > Move this method definition up above AddResultsForHTTPFallback. Done. https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:316: for resp in self.IterResponses(tab): On 2015/01/22 23:15:14, sclittle wrote: > What do you actually expect to happen here? Should the DRP return a safebrowsing > response, and the client should ignore it, or should the real response come in > from the DRP, or should the DRP cause a bypass? Or should the client not send > any request at all and pop up an interstitial? > > Please comment about what is expected to happen on desktop. Done. https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:319: safebrowsing_count += 1 On 2015/01/22 23:15:14, sclittle wrote: > Why is this called safebrowsing_count if you're counting via headers? Changed to viaheader_count. Had originally kept it safebrowsing_count because via header is the expected response for a site with malware in it for desktop. Don't have a strong preference either way. https://codereview.chromium.org/820093002/diff/20001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:328: results.current_page, 'safebrowsing', 'boolean', True)) On 2015/01/22 23:15:14, sclittle wrote: > Instead of just a boolean, how about adding both count and safebrowsing_count as > results? You could update the regular safebrowsing test to report these as well. Done. Added count. safebrowsing_count is redundant since this only gets printed when they are equal.
Sorry in advance about leaving so many comments on the old safebrowsing test. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:163: self._expect_timeout = False The default is to not expect a timeout, you could just remove this overridden WillNavigateToPage method altogether https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:287: if count == safebrowsing_count: The for loop above already raises an exception if any response is not a safebrowsing response, so count will always equal safebrowsing_count. You could remove the count variable altogether, and change this if statement to check that safebrowsing_count > 0. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:289: results.current_page, 'safebrowsing', 'url requests', count)) For consistency, the units here for this ScalarValue should be 'count'. Change to: results.AddValue(scalar.ScalarValue( results.current_page, 'safebrowsing', 'count', safebrowsing_count)) https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:292: 'Safebrowsing failed (count=%d, safebrowsing_count=%d)\n' % ( nit: change this message to be more descriptive (e.g. "No safebrowsing response was received") https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:296: count = 0 nit: maybe rename to response_count https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:300: # For desktop, Flywheel should return the real response for sites with s/Flywheel/the data reduction proxy/ https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:310: if count == viaheader_count: This will always be true, because the for loop would raise an exception if any of the responses don't have a via header. You should remove the count variable, and instead check that safebrowsing_count > 0 here. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:312: results.current_page, 'safebrowsing', 'url requests', count)) Change the last two parameters to 'count' and safebrowsing_count, for consistency with the other tests. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:315: 'Safebrowsing failed (count=%d, viaheader_count=%d)\n' % ( Change this failure message to be more descriptive
https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right): https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:163: self._expect_timeout = False On 2015/01/23 19:01:57, sclittle wrote: > The default is to not expect a timeout, you could just remove this overridden > WillNavigateToPage method altogether Done. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:287: if count == safebrowsing_count: On 2015/01/23 19:01:58, sclittle wrote: > The for loop above already raises an exception if any response is not a > safebrowsing response, so count will always equal safebrowsing_count. You could > remove the count variable altogether, and change this if statement to check that > safebrowsing_count > 0. Done. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:289: results.current_page, 'safebrowsing', 'url requests', count)) On 2015/01/23 19:01:58, sclittle wrote: > For consistency, the units here for this ScalarValue should be 'count'. Change > to: > > results.AddValue(scalar.ScalarValue( > results.current_page, 'safebrowsing', 'count', safebrowsing_count)) Discussed in person. Calling it 'count' everywhere does not give any additional information, so might be better to get rid of it entirely:) https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:292: 'Safebrowsing failed (count=%d, safebrowsing_count=%d)\n' % ( On 2015/01/23 19:01:58, sclittle wrote: > nit: change this message to be more descriptive (e.g. "No safebrowsing response > was received") Done. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:296: count = 0 On 2015/01/23 19:01:58, sclittle wrote: > nit: maybe rename to response_count Done. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:300: # For desktop, Flywheel should return the real response for sites with On 2015/01/23 19:01:57, sclittle wrote: > s/Flywheel/the data reduction proxy/ Done. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:310: if count == viaheader_count: On 2015/01/23 19:01:58, sclittle wrote: > This will always be true, because the for loop would raise an exception if any > of the responses don't have a via header. You should remove the count variable, > and instead check that safebrowsing_count > 0 here. Done. https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:312: results.current_page, 'safebrowsing', 'url requests', count)) On 2015/01/23 19:01:58, sclittle wrote: > Change the last two parameters to 'count' and safebrowsing_count, for > consistency with the other tests. :) https://codereview.chromium.org/820093002/diff/80001/tools/chrome_proxy/integ... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:315: 'Safebrowsing failed (count=%d, viaheader_count=%d)\n' % ( On 2015/01/23 19:01:58, sclittle wrote: > Change this failure message to be more descriptive Done.
LGTM with a couple nits https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:286: 'Safebrowsing failed: No valid responses received)\n') nit: remove ')\n' from the end https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:307: 'Safebrowsing failed: No valid responses received\n') nit: remove '\n' from the end
Let's agree on a naming strategy and then I'll approve. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:91: @benchmark.Enabled('android') sclittle: Which of these tests run with webview? kundaji: Should some of these be disabled on that platform? If you want to leave it as is, please list the platforms that are included in mobile and enable the benchmark on all mobile platforms. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:93: tag = 'safebrowsingmobile' Should we make this 'safebrowsingandroid'? I think we'll wind up having some tests enabled/disabled on each of our platforms. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:94: test = measurements.ChromeProxySafebrowsingMobile Likewise, "Mobile" or "Android"? https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py (right): https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py:58: # Page that should cause a bypass for linux clients. Linux
https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:91: @benchmark.Enabled('android') On 2015/01/23 20:18:19, bengr wrote: > sclittle: Which of these tests run with webview? > > kundaji: Should some of these be disabled on that platform? If you want to leave > it as is, please list the platforms that are included in mobile and enable the > benchmark on all mobile platforms. If I remember correctly, the @benchmark.Enabled('android') applies to webview as well as android chrome. We manually specify which tests should run for each platform in the jsons for the buildbots.
https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:91: @benchmark.Enabled('android') On 2015/01/23 20:18:19, bengr wrote: > sclittle: Which of these tests run with webview? > > kundaji: Should some of these be disabled on that platform? If you want to leave > it as is, please list the platforms that are included in mobile and enable the > benchmark on all mobile platforms. Gave more generic names. Lets deal with webview in another cl since it involves a different set of files. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:93: tag = 'safebrowsingmobile' On 2015/01/23 20:18:19, bengr wrote: > Should we make this 'safebrowsingandroid'? I think we'll wind up having some > tests enabled/disabled on each of our platforms. This test applies to iOS as well. Changed to "On" and "Off" and added comments. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:94: test = measurements.ChromeProxySafebrowsingMobile On 2015/01/23 20:18:19, bengr wrote: > Likewise, "Mobile" or "Android"? Acknowledged. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:286: 'Safebrowsing failed: No valid responses received)\n') On 2015/01/23 20:06:46, sclittle wrote: > nit: remove ')\n' from the end Done. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:307: 'Safebrowsing failed: No valid responses received\n') On 2015/01/23 20:06:46, sclittle wrote: > nit: remove '\n' from the end Done. https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py (right): https://codereview.chromium.org/820093002/diff/100001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/client_type.py:58: # Page that should cause a bypass for linux clients. On 2015/01/23 20:18:19, bengr wrote: > Linux Done.
https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:93: class ChromeProxySafeBrowsingOn(benchmark.Benchmark): Why not "enabled" like the tag below? https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:100: class ChromeProxySafeBrowsingOff(benchmark.Benchmark): Why not "disabled" like the tag below?
https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:93: class ChromeProxySafeBrowsingOn(benchmark.Benchmark): On 2015/01/23 22:59:41, sclittle wrote: > Why not "enabled" like the tag below? See below. https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:100: class ChromeProxySafeBrowsingOff(benchmark.Benchmark): On 2015/01/23 22:59:41, sclittle wrote: > Why not "disabled" like the tag below? I have seen "Disabled" being used for tests which have been temporarily disabled. Wanted to eliminate that ambiguity.
https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:100: class ChromeProxySafeBrowsingOff(benchmark.Benchmark): On 2015/01/23 23:03:28, kundaji wrote: > On 2015/01/23 22:59:41, sclittle wrote: > > Why not "disabled" like the tag below? > > I have seen "Disabled" being used for tests which have been temporarily > disabled. Wanted to eliminate that ambiguity. OK, maybe the tag should be "safebrowsing_off" then for consistency?
https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/120001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:100: class ChromeProxySafeBrowsingOff(benchmark.Benchmark): On 2015/01/23 23:10:34, sclittle wrote: > On 2015/01/23 23:03:28, kundaji wrote: > > On 2015/01/23 22:59:41, sclittle wrote: > > > Why not "disabled" like the tag below? > > > > I have seen "Disabled" being used for tests which have been temporarily > > disabled. Wanted to eliminate that ambiguity. > > OK, maybe the tag should be "safebrowsing_off" then for consistency? Done.
lgtm. Please add more detail to the CL description. Include what had to change to support integration tests for desktop and SB.
https://codereview.chromium.org/820093002/diff/180001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/180001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:157: return 'chrome_proxy_benchmark.safebrowsing.safebrowsing' This should be 'safebrowsing_on' https://codereview.chromium.org/820093002/diff/180001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:165: page_set = pagesets.SafebrowsingPageSet This class needs that def Name(cls) method as well, for safebrowsing_off
https://codereview.chromium.org/820093002/diff/180001/tools/chrome_proxy/inte... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/820093002/diff/180001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:157: return 'chrome_proxy_benchmark.safebrowsing.safebrowsing' On 2015/01/26 18:21:38, sclittle wrote: > This should be 'safebrowsing_on' Done. https://codereview.chromium.org/820093002/diff/180001/tools/chrome_proxy/inte... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:165: page_set = pagesets.SafebrowsingPageSet On 2015/01/26 18:21:38, sclittle wrote: > This class needs that def Name(cls) method as well, for safebrowsing_off Done.
On 2015/01/26 18:17:51, bengr wrote: > lgtm. Please add more detail to the CL description. Include what had to change > to support integration tests for desktop and SB. Done
On 2015/01/26 18:44:02, kundaji wrote: > On 2015/01/26 18:17:51, bengr wrote: > > lgtm. Please add more detail to the CL description. Include what had to change > > to support integration tests for desktop and SB. > > Done LGTM fwiw
The CQ bit was checked by kundaji@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/820093002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/58ee32e05d510fff9190c5f05fc9b0ea0df31ac1 Cr-Commit-Position: refs/heads/master@{#313098} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
