Deflake origin trial token validator tests

Deflake origin trial token validator tests

This removes the dependency on the system clock of the origin trial
token validator tests. All static functions in the TrialTokenValidator
namespace which validate tokens now take a base::Time argument which is used to
compare against the tokens provided.

BUG=672294
R=chasej@chromium.org,nhiroki@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_n5x_swarming_rel

Review-Url: https://codereview.chromium.org/2555333006
Cr-Commit-Position: refs/heads/master@{#491601}
Committed: https://chromium.googlesource.com/chromium/src/+/334e05fb8a8035585a25b1ce419425ac64b3889a

[iclelland, 2016-12-09 16:59:24]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 16:59:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2016-12-09 17:02:02]

Description was changed from

==========
Deflake origin trial token validator tests

This removes the dependency on the system clock of the origin trial
token validator tests. All static functions in the TrialTokenValidator
namespace which validate tokens now take a base::Time argument which is used to
compare against the tokens provided.

BUG=672294
R=chasej@chromium.org,nhiroki@chromium.org
==========

to

==========
Deflake origin trial token validator tests

This removes the dependency on the system clock of the origin trial
token validator tests. All static functions in the TrialTokenValidator
namespace which validate tokens now take a base::Time argument which is used to
compare against the tokens provided.

BUG=672294
R=chasej@chromium.org,nhiroki@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_n5x_swarming_rel
==========

[iclelland, 2016-12-09 18:01:15]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 18:01:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2016-12-09 18:03:36]

+r chasej for Origin Trials
+r nhiroki for content/browser

Can you PTAL? Thanks!

nhiroki, I'm not certain that base::Time::Now() is ideal in
c/b/service_worker/*, although it matches the existing behaviour. Do you know if
there's an actual base::Time::Clock that I should be using here instead?

[Marijn Kruisselbrink, 2016-12-09 19:24:07]

On 2016/12/09 at 18:03:36, iclelland wrote:
> nhiroki, I'm not certain that base::Time::Now() is ideal in
c/b/service_worker/*, although it matches the existing behaviour. Do you know if
there's an actual base::Time::Clock that I should be using here instead?

For bug #672636 we'll need something more than using base::Time::Now() directly.
At least some way to pass the base::Time to SetMainScriptHttpResponseInfo,
although maybe at that point it makes more sense to just add a
ServiceWorkerVersion::SetClockForTesting() analogous to the existing
SetTickClockForTesting, and use that clock in both these places?

[commit-bot: I haz the power, 2016-12-09 20:11:11]

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

[commit-bot: I haz the power, 2016-12-09 20:11:12]

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

[nhiroki, 2016-12-12 02:17:11]

nhiroki@chromium.org changed reviewers:
+ horo@chromium.org, mek@chromium.org

[nhiroki, 2016-12-12 02:20:51]

+mek@ and horo@ to the reviewer list.

They would be more appropriate reviewers for OriginTrial among service workers
(mek@ has worked on OT for Foreign Fetch and horo@ has worked on OT for
Navigation Prefetch).

[horo, 2017-01-05 08:07:58]

This cl itself LGTM.

ServiceWorkerVersion::SetClockForTesting() could be a good solution.
But I don't have a strong motivation to make all tests pass on "broken" bots.

[Marijn Kruisselbrink, 2017-01-05 22:39:53]

On 2017/01/05 at 08:07:58, horo wrote:
> This cl itself LGTM.
> 
> ServiceWorkerVersion::SetClockForTesting() could be a good solution.
> But I don't have a strong motivation to make all tests pass on "broken" bots.

I still think it makes more sense that if we're going to make any tests pass on
bots with broken clocks we should address all cases (so including a
SetClockForTesting on SWVersion and also fixing bug 672636), and not just fix
some of the tests while leaving those others flaky.

[iclelland, 2017-08-02 19:22:47]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-02 19:22:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-08-02 19:50:55]

On 2017/01/05 22:39:53, Marijn Kruisselbrink wrote:
> On 2017/01/05 at 08:07:58, horo wrote:
> > This cl itself LGTM.
> > 
> > ServiceWorkerVersion::SetClockForTesting() could be a good solution.
> > But I don't have a strong motivation to make all tests pass on "broken"
bots.
> 
> I still think it makes more sense that if we're going to make any tests pass
on
> bots with broken clocks we should address all cases (so including a
> SetClockForTesting on SWVersion and also fixing bug 672636), and not just fix
> some of the tests while leaving those others flaky.

mek@, does this latest patch look reasonable?

[Marijn Kruisselbrink, 2017-08-02 19:57:25]

looks reasonable, yes

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:90:
version_->SetClockForTesting(base::WrapUnique(clock));
why WrapUnique here, and not simply base::MakeUnique where you create the
instance?

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:295:
clock_(base::WrapUnique(new base::DefaultClock)),
here too, why WrapUnique and not just base::MakeUnique<base::DefaultClock>() ?

[iclelland, 2017-08-02 20:21:49]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-02 20:21:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-08-02 20:22:05]

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:90:
version_->SetClockForTesting(base::WrapUnique(clock));
On 2017/08/02 19:57:25, Marijn Kruisselbrink wrote:
> why WrapUnique here, and not simply base::MakeUnique where you create the
> instance?

Because I need to call SetNow() on it -- if I make it a unique_ptr<Clock>, then
I have to do a cludgy cast on the pointer immediately to call SimpleTestClock
methods -- and if I make a unique_ptr<SimpleTestClock>, then I can't pass that
to SetClockForTesting, since std::unique_ptr<base::SimpleTestClock> is not a
subclass of std::unique_ptr<base::Clock>

(On the other hand, it turns out that I can std::move() the test clock, and the
compiler is okay with that -- does that seem better than a raw pointer
temporary?)

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:295:
clock_(base::WrapUnique(new base::DefaultClock)),
On 2017/08/02 19:57:25, Marijn Kruisselbrink wrote:
> here too, why WrapUnique and not just base::MakeUnique<base::DefaultClock>() ?

Sigh.. I suppose because I can never remember which of {WrapUnique|MakeUnique}
is preferred without looking :(

Fixed. (And tick_clock_, too, for good measure)

[Marijn Kruisselbrink, 2017-08-02 20:34:20]

lgtm

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2555333006/diff/40001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:90:
version_->SetClockForTesting(base::WrapUnique(clock));
On 2017/08/02 at 20:22:05, iclelland wrote:
> (On the other hand, it turns out that I can std::move() the test clock, and
the compiler is okay with that -- does that seem better than a raw pointer
temporary?)

Yeah, that's what I meant. std::unique_ptr<Foo> is implicitly convertible to
std::unique_ptr<Bar> iff Foo* is implicitly convertible to Bar*

[commit-bot: I haz the power, 2017-08-02 23:20:52]

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

[commit-bot: I haz the power, 2017-08-02 23:20:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[iclelland, 2017-08-03 00:03:54]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-08-03 00:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2555333006/#ps60001
(title: "Switch WrapUnique to MakeUnique")

[commit-bot: I haz the power, 2017-08-03 00:04:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 01:39:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501718634470650,
"parent_rev": "523c40fa8b9b0cf09848557f280bb740b8e0fc6f", "commit_rev":
"18f72418c452cc171274dfaf630d6713a748a5d1"}

[commit-bot: I haz the power, 2017-08-03 01:39:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501718634470650,
"parent_rev": "7687d9be707990b4352d3a400d27c52c400e4eea", "commit_rev":
"334e05fb8a8035585a25b1ce419425ac64b3889a"}

[commit-bot: I haz the power, 2017-08-03 01:40:01]

Description was changed from

==========
Deflake origin trial token validator tests

This removes the dependency on the system clock of the origin trial
token validator tests. All static functions in the TrialTokenValidator
namespace which validate tokens now take a base::Time argument which is used to
compare against the tokens provided.

BUG=672294
R=chasej@chromium.org,nhiroki@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_n5x_swarming_rel
==========

to

==========
Deflake origin trial token validator tests

This removes the dependency on the system clock of the origin trial
token validator tests. All static functions in the TrialTokenValidator
namespace which validate tokens now take a base::Time argument which is used to
compare against the tokens provided.

BUG=672294
R=chasej@chromium.org,nhiroki@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_n5x_swarming_rel

Review-Url: https://codereview.chromium.org/2555333006
Cr-Commit-Position: refs/heads/master@{#491601}
Committed:
https://chromium.googlesource.com/chromium/src/+/334e05fb8a8035585a25b1ce4194...
==========

[commit-bot: I haz the power, 2017-08-03 01:40:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/334e05fb8a8035585a25b1ce4194...