[chrome_elf] blacklist flaky tests

[chrome_elf] blacklist flaky tests

Possible flake fix.  Overriding chrome_elf test launch options.

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2861793003
Cr-Commit-Position: refs/heads/master@{#470086}
Committed: https://chromium.googlesource.com/chromium/src/+/b1d09797bdb4b4b4340662a5bc25a0d611cc42ba

[penny, 2017-05-04 15:48:15]

Description was changed from

==========
[chrome_elf] blacklist flaky tests

BUG=
==========

to

==========
[chrome_elf] blacklist flaky tests

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[penny, 2017-05-04 15:49:50]

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

[commit-bot: I haz the power, 2017-05-04 15:50:08]

Dry run: 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-05-04 16:21:57]

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

[commit-bot: I haz the power, 2017-05-04 16:21:58]

Dry run: This issue passed the CQ dry run.

[penny, 2017-05-04 17:54:12]

Description was changed from

==========
[chrome_elf] blacklist flaky tests

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[chrome_elf] blacklist flaky tests

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[penny, 2017-05-04 17:54:56]

Description was changed from

==========
[chrome_elf] blacklist flaky tests

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[chrome_elf] blacklist flaky tests

Possible flake fix.

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[penny, 2017-05-04 18:27:32]

pennymac@chromium.org changed reviewers:
+ robertshield@chromium.org

[penny, 2017-05-04 18:57:41]

Hey Robert,

I'm taking a stab at fixing the flaky blacklist test.

Only run these one at a time - using a global mutex.

I've not seen this failure locally or on the try bots.
I'd like to throw this up and see if the waterfall sees any more flakes.

[grt (UTC plus 2), 2017-05-04 21:34:30]

drive-by

On 2017/05/04 18:57:41, penny wrote:
> Hey Robert,
> 
> I'm taking a stab at fixing the flaky blacklist test.
> 
> Only run these one at a time - using a global mutex.

May I humbly suggest making the whole test fixture run all tests serially? This
is conceptually much simpler, so unless there are a bajillion tests that need to
be parallelized, maybe it's the better option? To do so change
base::LaunchUnitTests in run_all_unittests.cc to LaunchUnitTestsWithOptions with
|default_jobs| set to 1. That might be all there is to it.

[penny, 2017-05-05 05:04:05]

On 2017/05/04 21:34:30, grt (UTC plus 2) wrote:
> drive-by
> 
> On 2017/05/04 18:57:41, penny wrote:
> > Hey Robert,
> > 
> > I'm taking a stab at fixing the flaky blacklist test.
> > 
> > Only run these one at a time - using a global mutex.
> 
> May I humbly suggest making the whole test fixture run all tests serially?
This
> is conceptually much simpler, so unless there are a bajillion tests that need
to
> be parallelized, maybe it's the better option? To do so change
> base::LaunchUnitTests in run_all_unittests.cc to LaunchUnitTestsWithOptions
with
> |default_jobs| set to 1. That might be all there is to it.

Oh wow.  I didn't realize there was a way to set a suite to serial!  I'll have a
look at that for just the blacklist* chrome_elf tests, if it's easy enough to
carve out a subset of this suite.

Thanks a lot Greg.  I appreciate your drive bys!

[grt (UTC plus 2), 2017-05-05 08:05:21]

On 2017/05/05 05:04:05, penny wrote:
> On 2017/05/04 21:34:30, grt (UTC plus 2) wrote:
> > drive-by
> > 
> > On 2017/05/04 18:57:41, penny wrote:
> > > Hey Robert,
> > > 
> > > I'm taking a stab at fixing the flaky blacklist test.
> > > 
> > > Only run these one at a time - using a global mutex.
> > 
> > May I humbly suggest making the whole test fixture run all tests serially?
> This
> > is conceptually much simpler, so unless there are a bajillion tests that
need
> to
> > be parallelized, maybe it's the better option? To do so change
> > base::LaunchUnitTests in run_all_unittests.cc to LaunchUnitTestsWithOptions
> with
> > |default_jobs| set to 1. That might be all there is to it.
> 
> Oh wow.  I didn't realize there was a way to set a suite to serial!  I'll have
a
> look at that for just the blacklist* chrome_elf tests, if it's easy enough to
> carve out a subset of this suite.
> 
> Thanks a lot Greg.  I appreciate your drive bys!

I think that the last few times this "serialize a few tests" issue had come up,
the advice has been that it isn't worth a subtle code solution in comparison to
serializing at the level of the test exe. Maybe check with the folks on
https://codereview.chromium.org/1027993002/ to see where they landed.

[grt (UTC plus 2), 2017-05-05 08:05:23]

On 2017/05/05 05:04:05, penny wrote:
> On 2017/05/04 21:34:30, grt (UTC plus 2) wrote:
> > drive-by
> > 
> > On 2017/05/04 18:57:41, penny wrote:
> > > Hey Robert,
> > > 
> > > I'm taking a stab at fixing the flaky blacklist test.
> > > 
> > > Only run these one at a time - using a global mutex.
> > 
> > May I humbly suggest making the whole test fixture run all tests serially?
> This
> > is conceptually much simpler, so unless there are a bajillion tests that
need
> to
> > be parallelized, maybe it's the better option? To do so change
> > base::LaunchUnitTests in run_all_unittests.cc to LaunchUnitTestsWithOptions
> with
> > |default_jobs| set to 1. That might be all there is to it.
> 
> Oh wow.  I didn't realize there was a way to set a suite to serial!  I'll have
a
> look at that for just the blacklist* chrome_elf tests, if it's easy enough to
> carve out a subset of this suite.
> 
> Thanks a lot Greg.  I appreciate your drive bys!

I think that the last few times this "serialize a few tests" issue had come up,
the advice has been that it isn't worth a subtle code solution in comparison to
serializing at the level of the test exe. Maybe check with the folks on
https://codereview.chromium.org/1027993002/ to see where they landed.

[grt (UTC plus 2), 2017-05-05 08:06:45]

On 2017/05/05 08:05:23, grt (UTC plus 2) wrote:
> On 2017/05/05 05:04:05, penny wrote:
> > On 2017/05/04 21:34:30, grt (UTC plus 2) wrote:
> > > drive-by
> > > 
> > > On 2017/05/04 18:57:41, penny wrote:
> > > > Hey Robert,
> > > > 
> > > > I'm taking a stab at fixing the flaky blacklist test.
> > > > 
> > > > Only run these one at a time - using a global mutex.
> > > 
> > > May I humbly suggest making the whole test fixture run all tests serially?
> > This
> > > is conceptually much simpler, so unless there are a bajillion tests that
> need
> > to
> > > be parallelized, maybe it's the better option? To do so change
> > > base::LaunchUnitTests in run_all_unittests.cc to
LaunchUnitTestsWithOptions
> > with
> > > |default_jobs| set to 1. That might be all there is to it.
> > 
> > Oh wow.  I didn't realize there was a way to set a suite to serial!  I'll
have
> a
> > look at that for just the blacklist* chrome_elf tests, if it's easy enough
to
> > carve out a subset of this suite.
> > 
> > Thanks a lot Greg.  I appreciate your drive bys!
> 
> I think that the last few times this "serialize a few tests" issue had come
up,
> the advice has been that it isn't worth a subtle code solution in comparison
to
> serializing at the level of the test exe. Maybe check with the folks on
> https://codereview.chromium.org/1027993002/ to see where they landed.

(Oh, and the reason I'm chiming in is that I implemented yet a different way a
while back. :-) )

[robertshield, 2017-05-05 13:04:43]

Chiming in slightly late to say that while the code lg, I like the approach Greg
suggested better.

https://codereview.chromium.org/2861793003/diff/20001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/2861793003/diff/20001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:42: #define event_timeout \
nit, mildly prefer: kEventTimeout or EVENT_TIMEOUT

[grt (UTC plus 2), 2017-05-05 13:11:47]

ROBERT MADE ME DO IT!!!!1!1

https://codereview.chromium.org/2861793003/diff/20001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/2861793003/diff/20001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:42: #define event_timeout \
On 2017/05/05 13:04:43, robertshield wrote:
> nit, mildly prefer: kEventTimeout or EVENT_TIMEOUT

if this can't be a true constant owing to TestTimeouts, then please use a
function -- macros are strongly discouraged:

DWORD SychronizationTimeout() {
  return static_cast<DWORD>(
      TestTimeouts::action_timeout().InMillisecondsRoundedUp());
}

or something along those lines.

https://codereview.chromium.org/2861793003/diff/20001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:45: const wchar_t*
g_blacklist_test_mutex = L"ChromeElfBlacklistTestMutex";
also from the nit factory:

constexpr wchar_t g_blacklist_test_mutex[] = ...;

is the preferred way to spell this since it ensures that the constant is in
read-only data and it retains the string length at compile in case anyone cares
to know about it.

[penny, 2017-05-06 16:54:01]

Patchset #3 (id:40001) has been deleted

[penny, 2017-05-06 16:54:20]

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

[commit-bot: I haz the power, 2017-05-06 16:54:51]

Dry run: 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-05-06 17:22:09]

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

[commit-bot: I haz the power, 2017-05-06 17:22:10]

Dry run: This issue passed the CQ dry run.

[penny, 2017-05-07 19:32:31]

Description was changed from

==========
[chrome_elf] blacklist flaky tests

Possible flake fix.

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[chrome_elf] blacklist flaky tests

Possible flake fix.  Overriding chrome_elf test launch options.

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[penny, 2017-05-07 19:33:40]

This looks cleaner.  chrome_elf_unittests is small enough that it's not much
slower to run serialized.

[penny, 2017-05-08 17:14:00]

Over to you Robert.

[robertshield, 2017-05-08 17:15:43]

lgtm

[penny, 2017-05-08 18:41:36]

The CQ bit was checked by pennymac@chromium.org

[commit-bot: I haz the power, 2017-05-08 18:42:51]

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-05-08 19:52:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494268896600190,
"parent_rev": "a881b3e6d19b6a38e1bd0118db8e993e40eac393", "commit_rev":
"b1d09797bdb4b4b4340662a5bc25a0d611cc42ba"}

[commit-bot: I haz the power, 2017-05-08 19:53:29]

Description was changed from

==========
[chrome_elf] blacklist flaky tests

Possible flake fix.  Overriding chrome_elf test launch options.

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[chrome_elf] blacklist flaky tests

Possible flake fix.  Overriding chrome_elf test launch options.

BUG=711651
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2861793003
Cr-Commit-Position: refs/heads/master@{#470086}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1d09797bdb4b4b4340662a5bc25...
==========

[commit-bot: I haz the power, 2017-05-08 19:53:30]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b1d09797bdb4b4b4340662a5bc25...

[jam, 2017-05-10 19:33:54]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/2869273005/ by jam@chromium.org.

The reason for reverting is: This is still causing lots of flake..