Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AltCover fails with: CancellationToken exists in both 'AltCover.Recorder.g' and 'System.Private.CoreLib' #133

Closed
samisq opened this issue Nov 15, 2021 · 11 comments
Labels

Comments

@samisq
Copy link

samisq commented Nov 15, 2021

We use Marten, a popular library for postgres database. Marten generates and compiles code at runtime. When running unit tests with AltCover against code the uses Marten, we get the following error.

CS0433: The type 'CancellationToken' exists in both 'AltCover.Recorder.g, Version=8.2.0.0, Culture=neutral, PublicKeyToken=4ebffcaabf10ce6a' and 'System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'

I have a very concise runnable repro here: https://github.com/samisq/AltCoverIssue. It requires running postgres server. You can run one in docker easily.

Repro steps:

$ git clone https://github.com/samisq/AltCoverIssue
$ docker run -d -p 5432:5432 --name some-postgres -e POSTGRES_PASSWORD=password postgres:alpine
$ dotnet test /p:Altcover=true

The issue started happening after build 7.1.782. So it doesn't occur on build 7.1.782 or earlier, or if AltCover is disabled.

I'm not interested in coverage for that generated code. So I wonder if there's a way to prevent instrumenting that code.

@SteveGilham
Copy link
Owner

Interesting. Looking into this, I'm surprised this one hasn't come up before.

After v7.1.782, in preparation for the net5.0 release, I reorganized the build so that rather than there being a separate .Net Framework and .Net Core build, there was one unified build; and the recorder became a shared net20-targeted assembly (with a suitably old FSharp.Core static linked at build time). It is that static linked code which is exposing the CancellationToken type (which used by other parts of that library).

If the Marten generated code builds a separate assembly, with a predictable name, then excluding that assembly (via /p:AltCoverAssemblyFilter, or /p:AltCoverAssemblyExcludeFilter in the case where the generated code assembly links against other code being instrumented, will prevent that code being given an reference to the recorder assembly, which may be enough resolve the issue.

If that is not the case, then from v7.2.800 a work-round exists in the form of using the /p:AltCoverCallContext to track the calls to any C# async method (it could be a dummy method that is not used by anything); this adds a small overhead to the collection phase in anything called by the named method, but, more importantly, switches in a net46 targeted build of the recorder needed to pick up the cross-thread context. That one links the net45-targeted version of the FSharp.Core, which can rely on the CancellationToken type being present in the exterior environment so does not expose it from the recorder.

For F# code, the equivalent work-round can be used from v8.1.817 with any method returning an async computation expression; again, it just has to be instrumented, it doesn't have to be used.

@samisq
Copy link
Author

samisq commented Nov 15, 2021

Thank you so much for the quick response. Marten generates and compiles the code at runtime, and uses random name for the assembly. So the exclude filters cannot work. But I tried the workaround with AltCoverCallContext and that worked!
I simply added <AltCoverCallContext>MyLib.Class1.Run</AltCoverCallContext> to my test's csproj.

I assume the assembly that contains the dummy async method must be instrumented, even if that method is never used. Is that correct?

@SteveGilham
Copy link
Owner

Yes, that's correct.

@SteveGilham
Copy link
Owner

SteveGilham commented Nov 15, 2021

Also, the exclude filtering may be able to do the job by either

  • taking advantage of the fact that the filters only have to match a sub-string of the assembly name, so any stable prefix or suffix can be used
  • taking advantage of the fact that the filters are regexes, and can thus be used to match names that fit a pattern, even if the actual characters that fill out the pattern are unpredictable
  • taking advantage of the inverted filters -- filter expressions with a leading ? are treated as excluding non-matching, so "exclude all strings that don't have substring 'xyzzy'" can be written as "?xyzzy" -- only for xyzzy put some string common to all assembly names in the project (typically the company or product name)

@samisq
Copy link
Author

samisq commented Nov 15, 2021

Thanks again for the tips. I actually already have an exclusion filter like this: <AltCoverAssemblyFilter>^(?!(MyLib)).*$</AltCoverAssemblyFilter> (I also tried AltCoverAssemblyExcludeFilter), but neither worked. I think that's because what's failing is the compilation of the generated code during the "normal" execution of the code, and not the instrumentation per se.

This what the stack track for the exception look like:

  Stack Trace:
   at LamarCompiler.AssemblyGenerator.Generate(String code)
   at LamarCompiler.AssemblyGenerator.Compile(GeneratedAssembly generatedAssembly, IServiceVariableSource services)
    ...
   at MyLib.Class1.Run(CancellationToken ct)

@SteveGilham
Copy link
Owner

Of course -- run-time created assemblies won't be affected by the filtering, but will be picking up whatever's in the process' environment. Well, that solidifies what the fix is going to have to be.

@SteveGilham
Copy link
Owner

To help avoid "works on my machine" effects, here's a pre-release build with a proposed fix for this issue, for the purpose of validation (nupkg inside zip archive to conform with file upload restrictions)

altcover.8.2.473-github-pre.nupkg.zip

This should work without needing the CallContext work-round.

@samisq
Copy link
Author

samisq commented Nov 16, 2021

I can confirm the issue is fixed in that build. Thank you so much for the quick turnaround!

@SteveGilham
Copy link
Owner

The issue was easy to diagnose, and the fix a simple one -- while rewriting the recorder prototype assembly to contain the session specific details such as the location of the report file, also make all the static-linked types internal. As I'd only just made one release and hadn't started on any new content, it was a quick win to just implement it.

The longest delay will be the wait for the next proper release, as, barring show-stoppers without work-round, I prefer to have some new content, and not just do releases with only a single bug-fix. That means it might not be until next week for the real deal.

@samisq
Copy link
Author

samisq commented Nov 16, 2021

That makes sense. Since we have a temporary workaround, waiting for the next release won't be a problem for us. At least we can upgrade to the current latest release now. Thank you again.

@SteveGilham
Copy link
Owner

Release 8.2.831 now out with this fix included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants