Add Finch testing configs for ReportCertificateErrors study

Add Finch testing configs for ReportCertificateErrors study

BUG=

Committed: https://crrev.com/fab211f362880f1df50f5d20b47c4a118ae6fc33
Cr-Commit-Position: refs/heads/master@{#339788}

[estark, 2015-07-20 20:46:08]

estark@chromium.org changed reviewers:
+ asvitkine@chromium.org

[estark, 2015-07-20 20:46:08]

asvitkine, could you please take a look? Thanks!

[Alexei Svitkine (slow), 2015-07-20 20:50:00]

lgtm

[estark, 2015-07-20 20:50:57]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-07-20 20:51:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244783006/1

[estark, 2015-07-20 21:42:57]

On 2015/07/20 20:50:00, Alexei Svitkine wrote:
> lgtm

Thanks Alexei. Now that the try bots are running, I'm seeing that this change
breaks tests where we try out different Finch groups and make sure that the
right thing happens in each different group
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...).
I thought these configs were only used for performance testing. Is there any way
we can have tests that override these configs?

[commit-bot: I haz the power, 2015-07-20 21:44:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-20 21:44:26]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[estark, 2015-07-20 21:49:29]

On 2015/07/20 21:42:57, estark wrote:
> On 2015/07/20 20:50:00, Alexei Svitkine wrote:
> > lgtm
> 
> Thanks Alexei. Now that the try bots are running, I'm seeing that this change
> breaks tests where we try out different Finch groups and make sure that the
> right thing happens in each different group
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...).
> I thought these configs were only used for performance testing. Is there any
way
> we can have tests that override these configs?

Ah, maybe the right thing to do is to add all the Finch configs that we want to
test (all the different groups) to fieldtrial_testing_config_*.json, and then
have the browser tests read the Finch params and adjust test expectations
accordingly. (Instead of having the browser tests set the Finch params.) Does
that sound about right?

[Alexei Svitkine (slow), 2015-07-20 22:02:44]

Indeed, we do want the configs to be used for testing - since one of the
benefits is this can uncover problems that you previously weren't aware of.

In terms of whether the test should check the experiment state or force
one, I don't think we have a best practice about this yet. So I suggest
doing whatever makes sense to you from the feature point of view - and
after some time of people trying this we can re-visit and see if it makes
sense to codify this in a best practice in our docs.

By the way, if you want your tests to check the experiment state, they
shouldn't read the JSON files themselves but instead use the same field
trial / variation params APIs as the code itself to modify their
expectations.

On Mon, Jul 20, 2015 at 5:49 PM, <estark@chromium.org> wrote:

> On 2015/07/20 21:42:57, estark wrote:
>
>> On 2015/07/20 20:50:00, Alexei Svitkine wrote:
>> > lgtm
>>
>
>  Thanks Alexei. Now that the try bots are running, I'm seeing that this
>> change
>> breaks tests where we try out different Finch groups and make sure that
>> the
>> right thing happens in each different group
>>
>
> (
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
> ).
>
>> I thought these configs were only used for performance testing. Is there
>> any
>>
> way
>
>> we can have tests that override these configs?
>>
>
> Ah, maybe the right thing to do is to add all the Finch configs that we
> want to
> test (all the different groups) to fieldtrial_testing_config_*.json, and
> then
> have the browser tests read the Finch params and adjust test expectations
> accordingly. (Instead of having the browser tests set the Finch params.)
> Does
> that sound about right?
>
> https://codereview.chromium.org/1244783006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[estark, 2015-07-21 05:33:00]

estark@chromium.org changed reviewers:
+ meacer@chromium.org

[estark, 2015-07-21 05:33:01]

Ok, makes sense. Could you please take another look, Alexei? I've updated to
include more configs.

meacer: could you please review //chrome/browser/ssl? The gist of this is that
we can now automatically have tests run with a bunch of different Finch configs,
instead of having to manually set the configs in the browser tests themselves.
So I've updated the tests to check if the Finch config says we should expect a
cert report, and then set the test expectations accordingly.

[meacer, 2015-07-21 06:04:31]

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:281:
IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest, CertReportingOptIn) {
nit: Can you add a comment here saying something like "this test should only run
if the expected report is OPT IN"? It's obvious from the code, but is also a bit
confusing given this one and the next test case almost do the same thing except
for the OPT_IN flag.

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.cc:179: if (sendingThreshold
== 1.0)
You might want to compare sendingThreshold against a slightly lower value such
as 1.0-epsilon here

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.h (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.h:41: const std::string&
GetLatestHostnameReported();
const method

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:1126: // Browser is incognito, user
proceeds, checkbox is shown and checked ->
Why do we show the checkbox in incognito if it doesn't control anything?

[estark, 2015-07-21 14:50:09]

Thanks, Mustafa.

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:281:
IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest, CertReportingOptIn) {
On 2015/07/21 06:04:31, Mustafa Emre Acer wrote:
> nit: Can you add a comment here saying something like "this test should only
run
> if the expected report is OPT IN"? It's obvious from the code, but is also a
bit
> confusing given this one and the next test case almost do the same thing
except
> for the OPT_IN flag.

Done. I also remove the Finch check from the next test, since I realized it's
actually even better to test no-opt-in => no-report under all the test Finch
configs.

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.cc:179: if (sendingThreshold
== 1.0)
On 2015/07/21 06:04:31, Mustafa Emre Acer wrote:
> You might want to compare sendingThreshold against a slightly lower value such
> as 1.0-epsilon here

I was actually hoping to try to discourage Finch configs that result in
non-deterministic behavior. Another alternative might be to check if the value
is between 0 and 1 (exclusive) and if so fail the test?

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.h (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.h:41: const std::string&
GetLatestHostnameReported();
On 2015/07/21 06:04:31, Mustafa Emre Acer wrote:
> const method

Done.

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:1126: // Browser is incognito, user
proceeds, checkbox is shown and checked ->
On 2015/07/21 06:04:31, Mustafa Emre Acer wrote:
> Why do we show the checkbox in incognito if it doesn't control anything?

Hmm, I think we don't, actually. "Checkbox is shown and checked" was probably
shorthand for "user is in Finch group where checkboxes get shown and had
previously opted-in in non-incognito so that the opt-in preference is set." But
the test is actually even a little more general than that now -- testing that
reports never get sent in incognito regardless of Finch config -- so I updated
the comment to match.

[meacer, 2015-07-21 17:36:25]

chrome/browser/ssl Lgtm

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_reporting_test_utils.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_reporting_test_utils.cc:179: if (sendingThreshold
== 1.0)
On 2015/07/21 14:50:09, estark wrote:
> On 2015/07/21 06:04:31, Mustafa Emre Acer wrote:
> > You might want to compare sendingThreshold against a slightly lower value
such
> > as 1.0-epsilon here
> 
> I was actually hoping to try to discourage Finch configs that result in
> non-deterministic behavior. Another alternative might be to check if the value
> is between 0 and 1 (exclusive) and if so fail the test?

Not sure if that check would be reliable either, but I don't see why it wouldn't
be.

Of course the other option is to let this go through and see if the tests fail
at all on trybots (they probably won't?)

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1244783006/diff/60001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:1126: // Browser is incognito, user
proceeds, checkbox is shown and checked ->
On 2015/07/21 14:50:09, estark wrote:
> On 2015/07/21 06:04:31, Mustafa Emre Acer wrote:
> > Why do we show the checkbox in incognito if it doesn't control anything?
> 
> Hmm, I think we don't, actually. "Checkbox is shown and checked" was probably
> shorthand for "user is in Finch group where checkboxes get shown and had
> previously opted-in in non-incognito so that the opt-in preference is set."
But
> the test is actually even a little more general than that now -- testing that
> reports never get sent in incognito regardless of Finch config -- so I updated
> the comment to match.

Ah, makes sense now.

[estark, 2015-07-21 22:28:14]

Alexei, I'm going to go ahead and let this land with the updated Finch configs
because they seem pretty straightforward and it already has your LGTM, but
please let me know if you have any objections and I'll be fine with reverting.

[estark, 2015-07-21 22:28:26]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-21 22:28:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1244783006/#ps80001
(title: "meacer comments")

[commit-bot: I haz the power, 2015-07-21 22:29:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244783006/80001

[commit-bot: I haz the power, 2015-07-21 22:52:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-21 22:52:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2015-07-21 22:55:16]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-21 22:55:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1244783006/#ps100001
(title: "fix GetLatestHostnameReported() const definition")

[commit-bot: I haz the power, 2015-07-21 22:56:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244783006/100001

[commit-bot: I haz the power, 2015-07-22 00:06:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-22 00:07:17]

Patchset 6 (id:??) landed as
https://crrev.com/fab211f362880f1df50f5d20b47c4a118ae6fc33
Cr-Commit-Position: refs/heads/master@{#339788}