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

Code coverage of System.Private.CoreLib is broken #51058

Closed
stephentoub opened this issue Apr 10, 2021 · 27 comments · Fixed by #64875
Closed

Code coverage of System.Private.CoreLib is broken #51058

stephentoub opened this issue Apr 10, 2021 · 27 comments · Fixed by #64875

Comments

@stephentoub
Copy link
Member

According to the directions, running a test suite like System.Runtime.Tests should be able to get coverage of the code in corelib via /p:Coverage=true /p:TestRuntime=true, but while doing so does add CoreLib to the command-line, the resulting code coverage report is empty.

cc: @ViktorHofer

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 10, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Apr 11, 2021

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

According to the directions, running a test suite like System.Runtime.Tests should be able to get coverage of the code in corelib via /p:Coverage=true /p:TestRuntime=true, but while doing so does add CoreLib to the command-line, the resulting code coverage report is empty.

cc: @ViktorHofer

Author: stephentoub
Assignees: -
Labels:

area-Infrastructure, untriaged

Milestone: -

@danmoseley
Copy link
Member

This has broken enough times, ideally we'd protect it with an outerloop test (just as we should protect launch and build in VS). As far as I know we have no existing example of a test that does a build, but presumably we can assume dotnet is on the path (at least we do in the build)

@stephentoub stephentoub added this to the 6.0.0 milestone Apr 11, 2021
@trylek
Copy link
Member

trylek commented Apr 12, 2021

For Helix executions of CoreCLR tests Simon added provisions for passing dotnet path around for the purpose of running Crossgen2, please see here:

os.environ["__TestDotNetCmd"] = args.dotnetcli_script_path

if [ ! -z ${__TestDotNetCmd+x} ] %3B then

I guess it shouldn't be that hard to introduce a similar logic to library tests.

@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Apr 21, 2021
@ViktorHofer
Copy link
Member

Marking as 6.0 and as a bug. Will look into this as soon as I have some cycles. Assigning to myself.

@ViktorHofer ViktorHofer self-assigned this Apr 21, 2021
@Anipik Anipik modified the milestones: 6.0.0, 7.0.0 Jul 21, 2021
@stephentoub
Copy link
Member Author

@danmoseley, is there someone who can look at this in Viktor's absence?

@danmoseley
Copy link
Member

@ericstj owns our infra backlog.

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 31, 2022

CoreLib code coverage measurement is broken because the following negative path is hit during coverlet's CanInstrument() check: https://github.com/coverlet-coverage/coverlet/blob/df968b862687aeb9ac86ae47938da04d95c10452/src/coverlet.core/Helpers/InstrumentationHelper.cs#L191.

coverlet.core.dll!Coverlet.Core.Helpers.InstrumentationHelper.MatchDocumentsWithSources(System.Reflection.Metadata.MetadataReader metadataReader) Line 191 C#
coverlet.core.dll!Coverlet.Core.Helpers.InstrumentationHelper.PortablePdbHasLocalSource(string module, out string firstNotFoundDocument) Line 162 C#
coverlet.core.dll!Coverlet.Core.Instrumentation.Instrumenter.CanInstrument() Line 108 C#
coverlet.core.dll!Coverlet.Core.Coverage.PrepareModules() Line 128 C#
coverlet.console.dll!Coverlet.Console.Program.Main.AnonymousMethod__1() Line 112 C#
[External Code]
coverlet.console.dll!Coverlet.Console.Program.Main(string[] args) Line 299 C#

Coverlet fails to find the generated ArayPoolEventSource file on disk: C:\git\runtime3\src\coreclr\System.Private.CoreLib\System.Private.CoreLib.Generators\Generators.EventSourceGenerator\ArrayPoolEventSource.Generated.cs.

The options that I see are:

  • Change coverlet's MatchDocumentsWithSources code path to also exclude ".Generated.cs" files or come up with a better heuristic. Potentially breaking customers.
  • Change the suffix of the generated code files in dotnet/runtime from .Generated.cs to .g.cs so that they are excluded by coverlet.

FWIW this was easy to debug by building coverlet myself, adding a Debugger.Attach() call and then invoking the coverlet.console.dll with the dotnet.exe from the testhost:
C:\git\runtime3\.dotnet\dotnet.exe C:\git\coverlet\src\coverlet.console\bin\Debug\net5.0\coverlet.console.dll "System.Runtime.Tests.dll" --target "C:\git\runtime3\artifacts\bin\testhost\net7.0-windows-Debug-x64\dotnet.exe" --targetargs " exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing" --format "opencover" --output "coverage.opencover.xml" --verbosity "normal" --use-source-link --exclude-by-file "C:\git\runtime3\src\libraries\Common\src\System\SR.*" --exclude-by-file "C:\git\runtime3\src\libraries\Common\src\System\NotImplemented.cs" --include-directory "C:\git\runtime3\artifacts\bin\testhost\net7.0-windows-Debug-x64\shared\Microsoft.NETCore.App\7.0.0" --include "[System.Runtime]*" --include "[System.Private.CoreLib]*"

I hope that helps :)

@stephentoub
Copy link
Member Author

stephentoub commented Jan 31, 2022

Thanks.

CoreLib code coverage measurement is broken because the following negative path is hit during coverlet's CanInstrument()

Fixing that (#64534) helps but does not solve the problem. Everything in corelib still shows as 0%.

image

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 31, 2022

I believe the remaining issue is that the instrumented IL code is at the wrong place:

	internal static void OnProcessExit()
	{
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20343);
		AssemblyLoadContext.OnProcessExit();
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20344);
		if (EventSource.IsSupported)
		{
			Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20345);
			Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20346);
			EventListener.DisposeOnShutdown();
		}
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20347);
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20348);
		EventHandler processExit = AppContext.ProcessExit;
		if (processExit == null)
		{
			Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20349);
		}
		else
		{
			Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20350);
			processExit(AppDomain.CurrentDomain, EventArgs.Empty);
			Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.RecordHitInCoreLibrary(20351);
			Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.UnloadModule(null, null);
		}
	}

This is from the ildasm my instrumented System.Private.CoreLib assembly. The code indicates that the processExit branch is hit and the Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_a5472523-6bb8-47da-bbb9-359dc829635b.UnloadModule method is never invoked and hence the result file is never written.

EDIT: The code responsible for injecting the UnloadModule method can be found here: https://github.com/coverlet-coverage/coverlet/blob/df968b862687aeb9ac86ae47938da04d95c10452/src/coverlet.core/Instrumentation/Instrumenter.cs#L303.

@stephentoub
Copy link
Member Author

@sharwell, seems that change came from coverlet-coverage/coverlet@96b1594. Can you take a look?

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 31, 2022

Yes that was it. If I change

        internal static void OnProcessExit()
        {
            AssemblyLoadContext.OnProcessExit();
            if (EventSource.IsSupported)
            {
                EventListener.DisposeOnShutdown();
            }


-            ProcessExit?.Invoke(AppDomain.CurrentDomain, EventArgs.Empty);
+           // Don't optimize this code path to inline the != null check via the ?. operator as otherwise
+           // coverlet can't inject the UnloadModule instrumentation code path and CoreLib's coverage results will be empty.
+           if (ProcessExit != null)
+           {
+               ProcessExit.Invoke(AppDomain.CurrentDomain, EventArgs.Empty);
+           }
        }
internal static void OnProcessExit()
{
	Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20343);
	AssemblyLoadContext.OnProcessExit();
	Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20344);
	if (EventSource.IsSupported)
	{
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20345);
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20346);
		EventListener.DisposeOnShutdown();
	}
	Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20347);
	Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20348);
	if (AppContext.ProcessExit != null)
	{
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20349);
		Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20350);
		AppContext.ProcessExit(AppDomain.CurrentDomain, EventArgs.Empty);
	}
	Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20351);
	Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.RecordHitInCoreLibrary(20352);
	Coverlet.Core.Instrumentation.Tracker.System.Private.CoreLib_0612f6fa-768c-4cd7-9fc6-d31e1f1280a5.UnloadModule(null, null);
}

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 31, 2022

There is one more remaining issue which unfortunately needs to be fixed in coverlet itself: coverlet-coverage/coverlet#1285. If I fix that locally and apply the AppContext.ProcessExit workaround from above I finally get code coverage results for CoreLib:

Calculating coverage result...
  Generating report 'C:\git\runtime3\artifacts\bin\System.Runtime.Tests\net7.0-Browser-Debug\coverage.opencover.xml'
+------------------------+--------+--------+--------+
| Module                 | Line   | Branch | Method |
+------------------------+--------+--------+--------+
| System.Private.CoreLib | 41.12% | 38.28% | 33.73% |
+------------------------+--------+--------+--------+
| System.Runtime         | 100%   | 100%   | 100%   |
+------------------------+--------+--------+--------+

+---------+--------+--------+--------+
|         | Line   | Branch | Method |
+---------+--------+--------+--------+
| Total   | 41.12% | 38.28% | 33.73% |
+---------+--------+--------+--------+
| Average | 70.56% | 69.14% | 66.86% |
+---------+--------+--------+--------+

Unfortunately I don't have any more time to help out here but I think that all the information needed is now on the table.

@jkotas
Copy link
Member

jkotas commented Jan 31, 2022

AppContext.ProcessExit workaround

Should this better be fixed in Coverlet too?

@danmoseley
Copy link
Member

Presumably this broke when we picked up the previous coverlet 1.7.2? If we ensure we always verify corelib coverage before updating our coverlet version, we should at least be protected against coverlet breaks, presumably?

Incidentally apparently this workaround can be removed now.
https://github.com/danmoseley/runtime/blob/9df283248256dd880eb1ee5ecce533ad206a55d3/eng/testing/coverage.targets#L62

@stephentoub
Copy link
Member Author

stephentoub commented Jan 31, 2022

Should this better be fixed in Coverlet too?

Yes 😄. cc: @sharwell, @MarcoRossignoli

@stephentoub
Copy link
Member Author

Unfortunately I don't have any more time to help out here but I think that all the information needed is now on the table.

Thanks for getting to it, Viktor.

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Jan 31, 2022

Should this better be fixed in Coverlet too?

I'll take a look asap(weekend I think).

Thanks @ViktorHofer 🙇

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Feb 5, 2022

I did a pair of fixes for the issues above, can someone with more "knowledge" and a ready runtime env(build etc...) give it a try before official release? I'm a bit rusty at the moment.

https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/ConsumeNightlyBuild.md

Will be shipped at midnight.

@stephentoub
Copy link
Member Author

Thanks, @MarcoRossignoli.

Will be shipped at midnight.

That's on the nightly feed. What about official builds?

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Feb 5, 2022

I do manual deploy for official build. I did one a few days ago 1/30...so the only difference between 3.1.1 and 3.1.2 will be this fix...for this reason I'd like to know before the release if it's working to avoid to ship an useful bugged version.
If it works I'll release asap(in few days).

I've fired manual dogfood release the version with the fix is 3.1.2-preview.4.gc8eafe5297 on https://pkgs.dev.azure.com/tonerdo/coverlet/_packaging/coverlet-nightly/nuget/v3/index.json

@stephentoub
Copy link
Member Author

I've fired manual dogfood release the version with the fix is 3.1.2-preview.4.gc8eafe5297

Tried it:

  Unhandled exception. System.InvalidProgramException: Common Language Runtime detected an invalid program.
     at System.AppContext.OnProcessExit()

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Feb 5, 2022

interesting...pls can you send here command list to build/run coverage, so I can go by my own

@stephentoub
Copy link
Member Author

stephentoub commented Feb 5, 2022

Apply stephentoub@009a5af to your dotnet/runtime repo clone.
(You might need to also dotnet tool restore after doing that.)
Build the repo: .\build clr+libs -rc release
Change into a test directory: cd src\libraries\System.Reflection.Emit\tests
Run tests with coverage: dotnet build /t:test /p:Coverage=true

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Feb 5, 2022

thx I'll be back asap(I have my suspect).

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Feb 5, 2022

looks good now I see the report(cannot evaluate the quality so if you see something strange let me know)
image

I'll ship the new version asap.

@MarcoRossignoli
Copy link
Member

Release 3.1.2 is out you can give it a try.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Feb 6, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants