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

Support for Deterministic Source Paths #405

Closed
clairernovotny opened this issue Jan 29, 2021 · 21 comments
Closed

Support for Deterministic Source Paths #405

clairernovotny opened this issue Jan 29, 2021 · 21 comments

Comments

@clairernovotny
Copy link

clairernovotny commented Jan 29, 2021

Today, it appears that ReportGenerator requires the "real" path to a file to work. If ContinousIntegrationBuild is set to true though, the paths in the report files should generate file paths of the like

/_/some/directory/file.cs
/_/some/directory/file2.cs
/_1/anotherRoot/frob/bar.cs

etc.

They'll always be normalized across OS versions.

What we'd need is for the sources parameter to search against all roots (ignore the /_{n}/) and try to find it against something like

C:\agent\1\work\s\some\directory\file.cs
C:\agent\1\work\s\some\directory\file2.cs

and match the files.

What can we do here?

Thanks!

@danielpalme
Copy link
Owner

I'm not sure I understand your problem correctly.

If you use the -sourcedirs command line parameter, ReportGenerator tries to locate the file by combining all the sourcedirectories, with all possible sub directories/files.

See:
https://github.com/danielpalme/ReportGenerator/blob/master/src/ReportGenerator.Core/Parser/FileReading/LocalFileReader.cs#L74

If you use -sourcedirs:C:\agent\1\work\s it will try to locate the file '/_/some/directory/file.cs` at these locations:

C:\agent\1\work\s\_\some\directory\file.cs
C:\agent\1\work\s\some\directory\file.cs
C:\agent\1\work\s\directory\file.cs
C:\agent\1\work\s\file.cs

The file C:\agent\1\work\s\some\directory\file.cs should be a match in your case.

Does this help?
If not, a concrete sample would help:

  • Coverage file
  • directory/file structure in your working directory
  • parameters passed to ReportGenerator

@clairernovotny
Copy link
Author

clairernovotny commented Jan 29, 2021

Providing the source directory did solve the warnings, but in each of the type files, it seems to be duplicating the overlay (and possibly not fully merging the line hit counts)?

Concretely, I have a result that looks like this with these two file names (same type, ByteArray):

/home/vsts/work/1/s/Collections/Specialized/ByteArray.cs
C:\a\1\s\Collections\Specialized\ByteArray.cs

The section of the report that shows the overlay is repeated twice, one for each file, so it's not clear that the line hit-counts are merged--what I want is to have those normalized and merged (they're the same file) to a single overlay.

@clairernovotny
Copy link
Author

clairernovotny commented Jan 29, 2021

@MarcoRossignoli seems like you can write out the deterministic path in the cobertura file instead of the local file path. That'll fix the merging issues. The only requirement then will be that -sourcedirs is always passed in to report generator since all paths will start with /_.../

@danielpalme it may be worth an either a specific error or some reasonable default if ReportGenerator detects that the path starts with /_ so things "just work" for users who fully enable deterministic builds.

@MarcoRossignoli
Copy link

Maybe on coverlet side we should add a flag like --deterministic-report to avoid to break other report generators.

@danielpalme
Copy link
Owner

@clairernovotny
Can you please share the coverage files containing the coverage information regarding ByteArray.cs?
I would like to debug this, to understand why merging does not work for you.
You can also send the files by email ([email protected]) if you don't want to share them publicly.

@danielpalme
Copy link
Owner

@clairernovotny
Thanks for your email. I will have a look within the next days.

@danielpalme
Copy link
Owner

I took a look at @clairernovotny files.

In your coverage files all paths start with:

  • C:\a\1\s\ (Windows)
  • /home/vsts/work/1/s/ (Linux)

I understand that the following paths actually mean the same file:

  • /home/vsts/work/1/s/Collections/Specialized/ByteArray.cs
  • C:\a\1\s\Collections\Specialized\ByteArray.cs

In the class coverage report generated by ReportGenerator, you would expect that e.g. for class ByteArray1, only one code file is listed.
That means, that ReportGenerator needs to treat both paths as the same file and only read one of the files from disk (the actual path depends on the operating system on which ReportGenerator is executed).

For me that means, we need a way to tell ReportGenerator that certain paths are mapped to other paths.
This could be done "manually" by specifying directory mappings (e.g. C:\a\1\s\ -> /home/vsts/work/1/s/) or by defining the most common path mappings (e.g. for Azure DevOps and GitHub Actions) and map them automatically (or maybe with a command line switch).

What I don't understand yet:

  • You mentioned paths starting with /_. Your coverage files don't contain any paths starting with /_
  • How does this relate to deterministic build? The paths should stay the same in a deterministic build? Or am I wrong here?

@clairernovotny
Copy link
Author

clairernovotny commented Jan 31, 2021

The files don't contain anything that starts with /_ today because Coverlet outputs the real path information into the coverage file instead of the deterministic path. Without any special parameters to ReportGenerator (passing in -sourcedirs:), that turns into a not-so-great user experience.

Coverlet could add an option as @MarcoRossignoli suggests in #405 (comment) where it'd write the deterministic file paths instead.

If they do that, on both Windows and Linux, the path would look something like

/_/Collections/Specialized/ByteArray.cs
/_/Collections/Specialized/CharArray.cs
or
/_1/OtherThingSourceRoot/SomeOtherThing/Foo.cs
/_2/YetAnotherSourceRoot/AnotherOtherThing/Bar.cs

In that case, ReportGenerator wouldn't need to do any special mapping of paths when matching types/files, but a user would have to pass in -sourcedirs: (I believe) so the search will work.

@danielpalme
Copy link
Owner

Thanks. Now I got it.

Adding support for this format should be easy in ReportGenerator.
The -sourcedirs parameter could also be optional. If no -sourcedirs are supplied, ReportGenerator can also check if the files exist at the default location for the most common CI/CD tools.

@MarcoRossignoli
Copy link

@danielpalme @clairernovotny you can try preview package of coverlet with deterministic report support https://f.feedz.io/marcorossignoli/coverletunofficial/nuget/index.json version 3.0.3-preview.6.ge4534aed18

-- deterministic build
dotnet build /p:Deterministic=true /p:ContinuousIntegrationBuild=true 

-- Collectors integration
dotnet test --no-build --collect:"XPlat Code Coverage" -- DataCollectionRunSettings.DataCollectors.DataCollector.Configuration.Format=cobertura DataCollectionRunSettings.DataCollectors.DataCollector.Configuration.DeterministicReport=true
-- Msbuild
dotnet test --no-build /p:DeterministicReport=true /p:CollectCoverage=true /p:CoverletOutputFormat=cobertura
-- Report generator
reportgenerator -reports:C:\git\coverletissue\DeterministicBuilds\MyTests\TestResults\438b7150-5a9a-4208-8d37-3f694ae90f75\coverage.cobertura.xml -targetdir:C:\git\coverletissue\DeterministicBuilds\MyTests\TestResults\438b7150-5a9a-4208-8d37-3f694ae90f75\ --sourcedirs:C:\git\coverletissue\DeterministicBuilds
<?xml version="1.0" encoding="utf-8"?>
<coverage line-rate="0.8571" branch-rate="0.5" version="1.9" timestamp="1612702997" lines-covered="6" lines-valid="7" branches-covered="1" branches-valid="2">
  <sources />
  <packages>
    <package name="MyLibrary" line-rate="0.8571" branch-rate="0.5" complexity="3">
      <classes>
        <class name="MyLibrary.Hello" filename="/_/MyLibrary/Hello.cs" line-rate="0.8571" branch-rate="0.5" complexity="3">
          <methods>
            <method name="get_Who" signature="()" line-rate="1" branch-rate="1" complexity="1">
              <lines>
                <line number="15" hits="1" branch="False" />
              </lines>
            </method>
            <method name=".ctor" signature="(System.String)" line-rate="0.8332999999999999" branch-rate="0.5" complexity="2">
              <lines>
                <line number="7" hits="1" branch="False" />
                <line number="8" hits="1" branch="False" />
                <line number="9" hits="1" branch="True" condition-coverage="50% (1/2)">
                  <conditions>
                    <condition number="16" type="jump" coverage="50%" />
                  </conditions>
                </line>
                <line number="10" hits="0" branch="False" />
                <line number="12" hits="1" branch="False" />
                <line number="13" hits="1" branch="False" />
              </lines>
            </method>
...

Expected result with current version of reportgenerator
image

@danielpalme
Copy link
Owner

@MarcoRossignoli
Thanks for the preview version of coverlt.
I will add the necessary code changes as soon as possible.
It may take some time, since I'm pretty busy.

@clairernovotny
Copy link
Author

@MarcoRossignoli I tried out that version and it seems to have worked!

@danielpalme
Copy link
Owner

@MarcoRossignoli
I just tried your previews:

  • coverlet.msbuild 3.0.3-preview.6.ge4534aed18
  • coverlet.collector 3.0.3-preview.6.ge4534aed18

With both packages, I get the full path path instead of the determistic path in coverage.cobertura.xml

Any idea what's wrong? Here is my test project:
coveragesample_msbuild.zip

@MarcoRossignoli
Copy link

MarcoRossignoli commented Feb 16, 2021

@danielpalme can you add this to CoverageSample project and retry(if the project is not under source control there is a missing source root)?

  <ItemGroup>
    <SourceRoot Include="$(MSBuildProjectDirectory)\" />
  </ItemGroup>
dotnet build /p:Deterministic=true /p:ContinuousIntegrationBuild=true
dotnet test --no-build /p:DeterministicReport=true /p:CollectCoverage=true /p:CoverletOutputFormat=cobertura
<?xml version="1.0" encoding="utf-8"?>
<coverage line-rate="1" branch-rate="1" version="1.9" timestamp="1613461225" lines-covered="3" lines-valid="3" branches-covered="0" branches-valid="0">
  <sources />
  <packages>
    <package name="CoverageSample" line-rate="1" branch-rate="1" complexity="1">
      <classes>
        <class name="CoverageSample.Calculator" filename="/_1/Calculator.cs" line-rate="1" branch-rate="1" complexity="1">
          <methods>
            <method name="Add" signature="(System.Int32,System.Int32)" line-rate="1" branch-rate="1" complexity="1">
              <lines>
                <line number="6" hits="1" branch="False" />
                <line number="7" hits="1" branch="False" />
                <line number="8" hits="1" branch="False" />
              </lines>
            </method>
          </methods>
          <lines>
            <line number="6" hits="1" branch="False" />
            <line number="7" hits="1" branch="False" />
            <line number="8" hits="1" branch="False" />
          </lines>
        </class>
      </classes>
    </package>
  </packages>
</coverage>

@danielpalme
Copy link
Owner

My repo is under source control.

When I add the <SourceRoot .../> element, the path now starts is /_1/Calculator.cs (as in your example)
This is not what I would expect. CoverageSample is missing in the path.

I tried with the following Directory.Build.props file in the solution directory:

<Project>

  <!-- SourceLink starts here. See https://github.com/dotnet/sourcelink -->
  <ItemGroup>
    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All"/>
  </ItemGroup>

  <PropertyGroup>
    <ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>
    <Deterministic>true</Deterministic>
  </PropertyGroup>

  <!-- Workaround. Remove once we're on 3.1.300+ https://github.com/dotnet/sourcelink/issues/572 -->
  <PropertyGroup>
    <TargetFrameworkMonikerAssemblyAttributesPath>$([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))</TargetFrameworkMonikerAssemblyAttributesPath>
  </PropertyGroup>
</Project>

Now i get the expected path: /_/CoverageSample/Calculator.cs

How does coverlet decide which path format is used in the XML?
I thought, deterministic paths should always start with /_/, but now /_1/ is used.
Are there any other formats like /_2/?

@clairernovotny
Copy link
Author

Deterministic paths can be anything that starts with /_. Apps may have multiple source roots (such as from source coming from NuGet packages). A submodule may be one too. Really anywhere the source may not share a common root.

@MarcoRossignoli
Copy link

MarcoRossignoli commented Feb 16, 2021

How does coverlet decide which path format is used in the XML?

It's not decided by coverlet, it's a msbuild sdk target that fill available roots.
Source link added that root I think or @clairernovotny can explain better(she helped a lot on this coverlet feature)

@clairernovotny
Copy link
Author

It comes from here: https://github.com/dotnet/roslyn/blob/8637000fc35f01a453349147b71d5ecdae8d0bff/src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets#L183-L199

@danielpalme
Copy link
Owner

I just added support for deterministic source paths in b0fd9b4.
If you work with Azure Devops with the default source directories (Windows: D:\a\1\s, Unix: /a/1/s) you won't have to specify the -sourcedirs parameter.

Before publishing an official release it would be helpful, if you could test a pre-release.
Should I publish a pre-release on Nuget?

@danielpalme
Copy link
Owner

Just made some additional changes 1350d0f and published release 4.8.6

@MarcoRossignoli
Copy link

Sorry for late response and thx @danielpalme.

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

No branches or pull requests

3 participants