Coverage for awaiting ValueTasks #949
Conversation
|
|
|
So, it turns out the fix for #914 works nicely in the Debug configuration, but not the Release configuration. The compiler-generated IL for |
|
Thanks for looking into the rabbit hole 😃 for tests please add also full coverage assertion like https://github.com/coverlet-coverage/coverlet/blob/master/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs#L18 You can use
Please add |
MarcoRossignoli
added
the
tenet-coverage
alexthornton1
changed the title
|
No worries. I had thought the work might be done, but I also thought there might be more to the story. I'll work on adding some higher-level tests and see where that leads. For now, I've changed the name of the PR to begin with [WIP]. I'll change it back when I think I'm done and we'll circle back again. Thanks for the response. |
First, I added a failing unit test, in two parts: * A new AsyncAwaitValueTaskStateMachine class in Samples.cs in coverlet.core.tests/Samples, in line with the existing AsyncAwaitStateMachine class, but returning a completed ValueTask instead of a completed Task. * A new test in CecilSymbolHelperTests.cs in coverlet.core.tests/Symbols that expects the "await" in that sample class not to have a branch in it. After it failed, I updated CecilSymbolHelper to include get_IsCompleted from System.Runtime.CompilerServices.ValueTaskAwaiter, as suggested by @a-jackson in issue coverlet-coverage#907. The test passed. So far, so good. Additionally, coverage testing was added. It was patterned from the existing coverage testing for awaiting Tasks, with a few differences: (1) The Async() method calls an overload of MemoryStream's ReadAsync() that returns a ValueTask. (2) ValueTask doesn't support ContinueWith(), so there's no coverage testing for calling ContinueWith(). (3) ValueTasks can be wrapped as Tasks, so there's coverage testing added for that.
alexthornton1
force-pushed
the
valuetask_await
branch
from
61e0d9f
to
5e13347
Compare
alexthornton1
changed the title
|
Let's pull these apart. Once I started adding coverage tests, the #907 fix worked fine, but #914 is a gnarlier beast than I had hoped. I'll still aim to attack it, but it seems clear it'll take longer, so no need to hold up the #907 fix -- which is what brought me here in the first place -- in the meantime. I moved #914 to another branch and will create a PR when I get a fuller solution together. So this PR is now just a #907 fix with coverage testing. Happy to add whatever else is necessary; this has been fun so far. |
Modified calls to GetAwaiter().GetResult() on a ValueTask<int> to use "await" instead. As it turns out, this is more of a stylistic change than anything else, as we're not actually dependent on the result of the calls to the various AsyncAwaitValueTask methods, since we're only interested in assessing coverage, but it's a good change nonetheless.
MarcoRossignoli
changed the title
For issue #907, first, I added a failing unit test, in two parts:
A new AsyncAwaitValueTaskStateMachine class in Samples.cs
in coverlet.core.tests/Samples, in line with the existing
AsyncAwaitStateMachine class, but returning a completed
ValueTask instead of a completed Task.
A new test in CecilSymbolHelperTests.cs in
coverlet.core.tests/Symbols that expects the "await" in
that sample class not to have a branch in it.
After it failed, I updated CecilSymbolHelper to include
get_IsCompleted from System.Runtime.CompilerServices.ValueTaskAwaiter,
as suggested by @a-jackson in issue #907. The test passed.
One open question is whether it might be worth adding a
ValueTask-based analogue for the Instrumentation.AsyncAwait.cs
samples.