Fix some missed hits

[lucaslorentz]

Updates and fixes some missed hits because it happened after hits file were written.

@ffMathy
Maybe you can review this.
Might not cover all edge cases, but fixes #103 and #126

[ffMathy]

I'll take a look soon! <3

[ffMathy]

Hmmm, it seems all good, but there's been quite a lot of changes, so I'm unsure what exactly the problem was, and what fixed it. Can you elaborate on that?

[lucaslorentz]

Renamed this to MethodScope and moved into a separate file. Because of this renaming the IL representation in all tests changed from MiniCover.HitServices.HitService/MethodContext to MiniCover.HitServices.MethodScope

[lucaslorentz]

It is not working as I expected, I will change probably change it a bit before explaining further

[lucaslorentz]

Some context.

For every IL instruction executed, we need to record it was hit.
So, every method is instrumented to with something like:

public void MyMethod() {
  MethodScope methodScope;
  try {
    methodScope = HitService.EnterMethod("MyAssembly", "MyClass", "MyMethod"); //We hardcode the string values during instrumentation
    methodScope.Hit(1); //We give every instruction an unique integer id, and record a hit before it is executed. The relationship between those IDs and the source files is saved in a "coverage.json" during instrumentation.
    ...first IL instruction
    methodScope.Hit(2);
    ...second IL instruction
    ...
  } finally {
    methodScope.Dispose();
  }
}

This instrumentation contract is solid enough to not be changed very often. So we can easily do fixes and change the behavior as long as HitContext.EnterMethod is still there and returns a MethodScope object with methods Hit and Dispose.

When your code run, we save all hit ids in folder coverage-hits. Then, we parse again the coverage.json + all hit ids and we are able to calculate any coverage information and generate any report.

The HitContext class is a container to store hits that were triggered by the same entry method.

One of the challenges of recording hits is when to write them to the disk.
It would be too slow to write them to disk on every hit and we wouldn't be able to aggregate and save only the totals.

We tried in the past to save hits to disk on AppDomain.CurrentDomain.DomainUnload event. But when the domain is unloading, your event handler has only about 2 seconds to execute, and it might not be enough to write all hits to disk. Also, if the application crashes and skips domain unloading you wouldn't write anything.

So, the approach we take now is to save the HitContext to disk when we exit the entry method.

That would work well, but not for async methods.
Because the async method is converted to a state machine class, and it is invoked several times until the state machine finishes.
So, a method like the below:

[Fact]
public async Task AsyncTest()
{
  await Task.Delay(100);
  var a = 1 + 2;
}

Will kind of be invoked twice. The first time will execute up to the await. The second time will run the rest of the method.
The error we have today is that both executions share the same HitContext, because it is propagated via AsyncLocal. But the HitContext is only written to disk after the first execution, and all hits from the second execution are lost.

The dirty solution implemented in this PR is to rewrite the full HitContext to disk after the second execution finishes. I also added a refcount, so in case there are multiple methods sharing the same HitContext in parallel, it will only rewrite to the disk when the last method finishes (refcount = 0).

I just realized maybe another fix would be to just replace AsyncLocal with just a static variable. So each time the state machine is invoked it creates a new HitContext, and writes it to disk once the state machine leaves.

[lucaslorentz]

@ffMathy
To do what you want regarding tracking hits per test method, you could either rely on our "entry" method concept.
But it will not be correct for hits invoked by test class constructors, dispose, tests setup, tear down, or when you have a single test method executed with different test data.

You could also try to get the running test from XUnit, Nunit... But some of them probably don't have mechanisms to know what's the running test.

[lucaslorentz]

This is still wrong. When it rewrites the HitContext to disk it will generate a new id and duplicate the hits in 2 files.

I will change it to make sure the same HitContext is always written to the same file. Maybe I can optimize as well to only write the diff the second time the hit context is saved.

[ffMathy]

Thank you for the elaborate description! I'll review once you've made the additional changes you're referring to.

In addition, I think it'd be quite effective to have some integration tests that actually compile a sample project, and then instrument the assemblies, run them, gather a report, and read that the values in the report are correct.

That way, we don't only just look at the IL - we also make sure it actually works in reality.

What do you think of this? If that sounds good, I'll probably add some integration tests to my next contribution 😊

[lucaslorentz]

Sounds good, but maybe do that in a separate PR.
Today we have https://github.com/lucaslorentz/minicover/blob/master/build-sample.sh
Which builds a sample project and run tests, this validates the instrumentation doesn't break the IL, but it doesn't check the coverage numbers.

[ffMathy]

Interesting.

By the way, can we use some of this? See the async state machine part:

https://dev.to/nikiforovall/awaitable-awaiter-pattern-and-logical-micro-threading-in-c-2701

[lucaslorentz]

@ffMathy
Interesting!
But I don't see how I can use that, maybe you have some idea?
All those tricks seem to be accomplished with the help of Roslyn compiler.
It would be a bit harder than that to change the GetAwaiter calls once it is IL.

[lucaslorentz]

The solution in this PR is now better than what we had before and will handle async as well.
I will merge and release a new version of Minicover.