Integration tests for desktop platforms and safebrowsing

tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py

Index: tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py
diff --git a/tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py b/tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py
index 8857a56e4cff95393360195f389fa1e3a96bc157..baaa3e16f09a450167aa68df3fb0cd74a42a6fd9 100644
--- a/tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py
+++ b/tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py
@@ -270,7 +270,7 @@ class ChromeProxyMetric(network_metrics.NetworkMetric):
       results.AddValue(scalar.ScalarValue(
           results.current_page, 'bypass', 'count', bypass_count))
 
-  def AddResultsForSafebrowsing(self, tab, results):
+  def AddResultsForSafebrowsingMobile(self, tab, results):
     count = 0
     safebrowsing_count = 0
 
@@ -286,12 +286,35 @@ class ChromeProxyMetric(network_metrics.NetworkMetric):
                 r.url, r.status, r.status_text, r.headers))
     if count == safebrowsing_count:

[sclittle, 2015/01/23 19:01:58]

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.

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

Done.

       results.AddValue(scalar.ScalarValue(
-          results.current_page, 'safebrowsing', 'boolean', True))
+          results.current_page, 'safebrowsing', 'url requests', count))

[sclittle, 2015/01/23 19:01:58]

For consistency, the units here for this ScalarValue should be 'count'. Change
to:

results.AddValue(scalar.ScalarValue(
    results.current_page, 'safebrowsing', 'count', safebrowsing_count))

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

Discussed in person. Calling it 'count' everywhere does not give any additional
information, so might be better to get rid of it entirely:)

     else:
       raise ChromeProxyMetricException, (
           'Safebrowsing failed (count=%d, safebrowsing_count=%d)\n' % (

[sclittle, 2015/01/23 19:01:58]

nit: change this message to be more descriptive (e.g. "No safebrowsing response
was received")

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

Done.

               count, safebrowsing_count))
 
+  def AddResultsForSafebrowsingDesktop(self, tab, results):
+    count = 0

[sclittle, 2015/01/23 19:01:58]

nit: maybe rename to response_count

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

Done.

+    viaheader_count = 0
+    for resp in self.IterResponses(tab):
+      count += 1
+      # For desktop, Flywheel should return the real response for sites with

[sclittle, 2015/01/23 19:01:57]

s/Flywheel/the data reduction proxy/

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

Done.

+      # malware.
+      if resp.HasChromeProxyViaHeader():
+        viaheader_count += 1
+      else:
+        r = resp.response
+        raise ChromeProxyMetricException, (
+            '%s: Safebrowsing feature should be off for desktop.\n'
+            'Reponse: status=(%d, %s)\nHeaders:\n %s' % (
+                r.url, r.status, r.status_text, r.headers))
+    if count == viaheader_count:

[sclittle, 2015/01/23 19:01:58]

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.

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

Done.

+      results.AddValue(scalar.ScalarValue(
+          results.current_page, 'safebrowsing', 'url requests', count))

[sclittle, 2015/01/23 19:01:58]

Change the last two parameters to 'count' and safebrowsing_count, for
consistency with the other tests.

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

:)

+    else:
+      raise ChromeProxyMetricException, (
+          'Safebrowsing failed (count=%d, viaheader_count=%d)\n' % (

[sclittle, 2015/01/23 19:01:58]

Change this failure message to be more descriptive

[Not at Google. Contact bengr, 2015/01/23 20:02:47]

Done.

+              count, viaheader_count))
+
   def AddResultsForHTTPFallback(self, tab, results):
     via_fallback_count = 0