Hmmm interesting, actually that re-check behind the lock is intentional... I guess we need a racy test to hit that case.

Not sure if that's the case of base/task_scheduler, but in general it might be problematic to get a pure 100% coverage due to issue 843209. However, we'll eventually find a solution for that, so just FYI.

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

commit 029d79cd5c7d38489ba843951f0ec47f213bf05a
Author: Gabriel Charette <gab@chromium.org>
Date: Tue May 22 16:08:12 2018

[TaskScheduler] Add a test to exercise AddDelayedTask() during Start().

https://chromium-coverage.appspot.com/reports/559420/linux/chromium/src/base/task_scheduler/delayed_task_manager.cc.html

Highlighted that we had untested code. The re-check behind the lock is
required to support AddDelayedTask() racing with Start(). Added a test
for this race which will hopefully trigger coverage here :)

21.5M  if (started_.IsSet()) {
21.2M    AddDelayedTaskNow(std::move(task), delay,
21.2M                      std::move(post_task_now_callback));
21.2M  } else {
301k    AutoSchedulerLock auto_lock(lock_);
301k    if (started_.IsSet()) {
0         AddDelayedTaskNow(std::move(task), delay,
0                           std::move(post_task_now_callback));
301k    } else {
301k      tasks_added_before_start_.push_back(
301k          {std::move(task), std::move(post_task_now_callback)});
301k    }
301k  }

R=fdoray@chromium.org

Bug: 844597
Change-Id: I7bd9f47e0c76db5f0a4309210db33cb746b83eeb
Reviewed-on: https://chromium-review.googlesource.com/1066621
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560626}
[modify] https://crrev.com/029d79cd5c7d38489ba843951f0ec47f213bf05a/base/task_scheduler/delayed_task_manager_unittest.cc

@#4, thanks for the FYI mmoroz@ : in base/task_scheduler we typically have death tests to exercise our DCHECKs, will this work?

Oh nvm, I see this is the case for us as well, e.g. https://chromium-coverage.appspot.com/reports/560344/linux/chromium/src/base/task_scheduler/lazy_task_runner.cc.html

Thanks!

@fdoray:

in lazy_task_runner.cc we have :

29  47.5k   // Return if no reference is held by this instance.
30  47.5k   if (!state)
31  0         return;

I guess we just need to test that double Reset() no-ops.

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

commit 260715d73a50435907a404e14f8eeed563d83b92
Author: Gabriel Charette <gab@chromium.org>
Date: Wed May 23 19:40:03 2018

[TaskScheduler] Remove unused Sequence::PeekTaskTraits()

R=fdoray@chromium.org

Bug: 844597
Change-Id: I9ee0e989c5e77922e7d6cc57cea3cab6d024966b
Reviewed-on: https://chromium-review.googlesource.com/1067651
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561209}
[modify] https://crrev.com/260715d73a50435907a404e14f8eeed563d83b92/base/task_scheduler/sequence.cc
[modify] https://crrev.com/260715d73a50435907a404e14f8eeed563d83b92/base/task_scheduler/sequence.h

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

commit 2ea8d0a1b24aca6e5a6e9e553d8a3555f1f83c3f
Author: Gabriel Charette <gab@chromium.org>
Date: Wed May 23 22:55:25 2018

[TaskScheduler] Delete unused operator in priority_queue.cc

R=fodray@chromium.org

Bug: 844597
Change-Id: I22a339568de6596335b64c947fbe87be060c469e
Reviewed-on: https://chromium-review.googlesource.com/1067646
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561294}
[modify] https://crrev.com/2ea8d0a1b24aca6e5a6e9e553d8a3555f1f83c3f/base/task_scheduler/priority_queue.cc

Turns out these were the only little things missing coverage :)

Everything else is because of the issues blocking this bug (DCHECKs are not covered, nor is code only running in dcheck builds, and code specific to Windows configuration isn't covered either).

DCHECKs are now covered. Can you evaluate results and report any issue you see. Once done, then feel free to put back to available (blocked on windows support).

Nice, that brings us to 97.57% :).

It still reports some DCHECKs as uncovered but I guess that's because they're never hit (i.e. we'd need more death tests).

Remaining things are :
 * a few one-liners (e.g. simple early no-op returns when doing an operation twice) which I'd missed when they were surrounded with missing DCHECK coverage.
 * code specific to Windows configuration (issue 809150)
 * code evaluated at compile-time (constexpr) (issue 845575)

Nothing high-priority but we could go up a few more points before being blocked again with thorough death tests and redundant tests (no-op on second call).