For an easy repro, run

$ ./out/Default/blimp_unittests --single-process-tests

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ce5fa0395e29192dcefe6d3211d947b32968d40

commit 1ce5fa0395e29192dcefe6d3211d947b32968d40
Author: scf <scf@chromium.org>
Date: Tue Oct 25 17:16:00 2016

Helium: Fix flaky test

Unit test fixture was not using HeliumTest test fixture and thus not properly cleaning state.

BUG= 658798 

Review-Url: https://codereview.chromium.org/2444193002
Cr-Commit-Position: refs/heads/master@{#427387}

[modify] https://crrev.com/1ce5fa0395e29192dcefe6d3211d947b32968d40/blimp/helium/compound_syncable_unittest.cc

The failure is now no longer reproducible with --single-process-tests, but I feel like the RevisionGeneratorTest.GetNextRevisionCall is still wrong -- it assumes that it's the first one in the whole binary to call GetNextRevision, which seems like a pretty brittle assumption -- if you add a second test, which is exactly the same, character for character, it will fail. Tests should be idempotent and isolated.

Also, I don't quite understand how the CL above fixes the failure -- could you explain to me how it works? From my brief glance, HeliumTest fixture doesn't seem to actually do anything...

HeliumTest uses base::ShadowingAtExitManager which takes care of cleaning up the state from GetNextRevisionCall.

>if you add a second test, which is exactly the same, character for character, it will fail. Tests should be idempotent and isolated.
False. 
Gtest will create a fresh test fixture so that ensures ShadowingAtExitManager is destructed on every test (See https://github.com/google/googletest/blob/master/googletest/docs/Primer.md).

Ah, I understand it now. ShadowingAtExitManager takes care of destroying the lazy instance keeping the global counter, so the test cleans up after itself, which is why running the same test twice in a row works correctly. If everything cleans up after using global GetNextRevision(), the test should continue working. I think it's still a bit brittle, but much less so than I though initially, and thus I'm not sure if it's worth spending any more time on this. Closing the issue.

And by the addition of OwnedRegisterTest, the GetNextRevision test is broken again. I think it proves that the current approach is brittle, and that it really requires fixing.

xyzzyz@xyzzyz:~/chromium/src$ out/Default/blimp_unittests --single-process-tests --gtest_filter=RevisionGeneratorTest.*:OwnedRegisterTest.*
Note: Google Test filter = RevisionGeneratorTest.*:OwnedRegisterTest.*
[==========] Running 8 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 5 tests from OwnedRegisterTest
[ RUN      ] OwnedRegisterTest.SetIncrementsLocalVersion
[       OK ] OwnedRegisterTest.SetIncrementsLocalVersion (0 ms)
[ RUN      ] OwnedRegisterTest.SyncSuccessfully
[       OK ] OwnedRegisterTest.SyncSuccessfully (0 ms)
[ RUN      ] OwnedRegisterTest.ChangesetIgnored
[       OK ] OwnedRegisterTest.ChangesetIgnored (0 ms)
[ RUN      ] OwnedRegisterTest.ChangesetsAppliedOutOfOrderFails
[       OK ] OwnedRegisterTest.ChangesetsAppliedOutOfOrderFails (0 ms)
[ RUN      ] OwnedRegisterTest.InvalidOperationForPeer
[       OK ] OwnedRegisterTest.InvalidOperationForPeer (0 ms)
[----------] 5 tests from OwnedRegisterTest (0 ms total)

[----------] 3 tests from RevisionGeneratorTest
[ RUN      ] RevisionGeneratorTest.CheckCurrentDoesntIncrease
[       OK ] RevisionGeneratorTest.CheckCurrentDoesntIncrease (0 ms)
[ RUN      ] RevisionGeneratorTest.MonotonicallyIncreasing
[       OK ] RevisionGeneratorTest.MonotonicallyIncreasing (0 ms)
[ RUN      ] RevisionGeneratorTest.GetNextRevisionCall
../../blimp/helium/revision_generator_unittest.cc:29: Failure
Value of: GetNextRevision()
  Actual: 5
Expected: 1UL
Which is: 1
../../blimp/helium/revision_generator_unittest.cc:30: Failure
Value of: GetNextRevision()
  Actual: 6
Expected: 2UL
Which is: 2
../../blimp/helium/revision_generator_unittest.cc:31: Failure
Value of: RevisionGenerator::GetInstance()->current()
  Actual: 6
Expected: 2UL
Which is: 2
[  FAILED  ] RevisionGeneratorTest.GetNextRevisionCall (1 ms)
[----------] 3 tests from RevisionGeneratorTest (1 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 2 test cases ran. (2 ms total)
[  PASSED  ] 7 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] RevisionGeneratorTest.GetNextRevisionCall

 1 FAILED TEST

Thats because OwnedRegisterTest didn't extend HeliumTest. @lethalantidote do you mind taking care of this?

Feels like this is kind of a band-aid. Work is in progress, but the RevisionGeneratorTest should not be effected by other test classes like this. 

Yes, I agree with that. I think that test should make sure to set up the environment it expects itself, or be updated to deal with the environment it finds.

The issue was that tests were missing AtExitManager (didn't derive from HeliumTest).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c69887e2547ece792256051d5fcce3f6d575b25d

commit c69887e2547ece792256051d5fcce3f6d575b25d
Author: lethalantidote <lethalantidote@chromium.org>
Date: Fri Nov 18 03:12:50 2016

OwnedRegisterTest now dervives from HeliumTest.

Updates test class to dervive from HeliumTest, which contributes
ShadowingAtExitManager.

BUG= 658798 

Review-Url: https://codereview.chromium.org/2470883002
Cr-Commit-Position: refs/heads/master@{#433046}

[modify] https://crrev.com/c69887e2547ece792256051d5fcce3f6d575b25d/blimp/helium/owned_register_unittest.cc