Skip to content

new code coverage #5169

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.

Sign up for GitHub

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

Merged
frank-dong-ms-zz merged 11 commits into from
May 30, 2020
Merged

new code coverage #5169

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
189cbda
test new code coverage
frank-dong-ms-zz May 27, 2020
a6fb2bc
change coverage path
frank-dong-ms-zz May 27, 2020
c66440c
fix failure
frank-dong-ms-zz May 27, 2020
fd8af95
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
frank-dong-ms-zz May 27, 2020
3aa52de
try test code coverage
frank-dong-ms-zz May 27, 2020
4136e38
fix build
frank-dong-ms-zz May 28, 2020
a55a2e0
try new file filters
frank-dong-ms-zz May 28, 2020
d7e608f
fix coverage report path
frank-dong-ms-zz May 29, 2020
19902f5
fix coverage report path
frank-dong-ms-zz May 29, 2020
4b5a213
try remove GC.Collect
frank-dong-ms-zz May 29, 2020
6ec9759
remove unnecessary new line
frank-dong-ms-zz May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions build.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
$(TraversalBuildDependsOn);
DownloadExternalTestFiles;
DownloadTensorflowMetaFiles;
DeleteTestHost;
</TraversalBuildDependsOn>
</PropertyGroup>

Expand Down Expand Up @@ -116,6 +117,18 @@
</DownloadFile>
</Target>

<!-- Delete testhost.dll and testhost.exe from output folder,
start test from dotnet.exe to keep consistent behavior with older version of Microsoft.NET.Test.Sdk -->
<Target Name="DeleteTestHost">
<Message Importance="High" Text="Delete testhost.dll and testhost.exe from output folder..." />

@harishsk harishsk May 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Why is this necessary?

@frank-dong-ms-zz frank-dong-ms-zz May 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also related to upgrade package "Microsoft.NET.Test.Sdk". Before upgrade the package, testhost.dll and testhost.exe are not copied to output path and tests are all starting from dotnet.exe. After upgrade package, tests will starts from testhost which will cause 2 issues:

  1. RemoteExecutor will not able to start new process as below line will get testhost instead of dotnet: https://github.com/dotnet/machinelearning/blob/master/test/Microsoft.ML.TestFramework/RemoteExecutor.cs#L29
  2. For x86 tests, we should use testhost.x86 instead of testhost directly, this should be achieved by set /p:Configuration instead of buildArch(which is ML.NET currently using), but setting Configuration will modify the build behavior a lot and corrupt the ML.NET build system.

So, based on these 2 reason, I'm add this target to remove testhost after build to keep consistent behavior as before upgrade test sdk package.

In reply to: 432262763 [](ancestors = 432262763)

<ItemGroup>
<FilesToClean Include="$(MSBuildThisFileDirectory)\bin\**\testhost.dll" />
<FilesToClean Include="$(MSBuildThisFileDirectory)\bin\**\testhost.exe" />
</ItemGroup>
<Delete Files="@(FilesToClean)"/>
<RemoveDir Directories="@(FoldersToClean)" />
</Target>

<Target Name="RunTests">
<MSBuild Projects="test\run-tests.proj"
Targets="RunTests" />
Expand Down
4 changes: 2 additions & 2 deletions build/Codecoverage.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
<_ReportGeneratorPath>$(PkgReportGenerator)\tools\net47\ReportGenerator.exe</_ReportGeneratorPath>
</PropertyGroup>

<Message Importance="high" Text="&quot;$(_ReportGeneratorPath)&quot; -reports:$(BaseOutputPath)$(PlatformConfig)\coverage\*.coverage -targetdir:$(BaseOutputPath)$(PlatformConfig)\coverage -filefilters:+https*;+*.fs -reporttypes:Cobertura" />
<Exec Command="&quot;$(_ReportGeneratorPath)&quot; -reports:$(BaseOutputPath)$(PlatformConfig)\coverage\*.coverage -targetdir:$(BaseOutputPath)$(PlatformConfig)\coverage -filefilters:+https*;+*.fs -reporttypes:Cobertura" />
<Message Importance="high" Text="&quot;$(_ReportGeneratorPath)&quot; -reports:$(BaseOutputPath)$(PlatformConfig)\*\*\coverage.opencover.xml -targetdir:$(BaseOutputPath)$(PlatformConfig)\coverage -filefilters:+*.cs;+*.fs -reporttypes:Cobertura" />
<Exec Command="&quot;$(_ReportGeneratorPath)&quot; -reports:$(BaseOutputPath)$(PlatformConfig)\*\*\coverage.opencover.xml -targetdir:$(BaseOutputPath)$(PlatformConfig)\coverage -filefilters:+*.cs;+*.fs -reporttypes:Cobertura" />

<ItemGroup>
<_CodecovArgs Include="-f;$(BaseOutputPath)$(PlatformConfig)\coverage\Cobertura.xml" />
Expand Down
2 changes: 1 addition & 1 deletion build/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
<PropertyGroup>
<PublishSymbolsPackageVersion>1.0.0-beta-62824-02</PublishSymbolsPackageVersion>
<CodecovVersion>1.9.0</CodecovVersion>
<CoverletVersion>2.7.0</CoverletVersion>
<CoverletCollectorVersion>1.2.1</CoverletCollectorVersion>
<ReportGeneratorVersion>4.3.6</ReportGeneratorVersion>
<MicrosoftDotNetApiCompatPackageVersion>1.0.0-beta.19225.5</MicrosoftDotNetApiCompatPackageVersion>
</PropertyGroup>
Expand Down
22 changes: 2 additions & 20 deletions test/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.8.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.6.1" />
<PackageReference Include="MSTest.TestFramework" Version="2.1.0" />
<PackageReference Include="MSTest.TestAdapter" Version="2.1.0" />
<PackageReference Include="xunit" Version="2.4.0" />
<PackageReference Include="Xunit.Combinatorial" Version="$(XunitCombinatorialVersion)" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.0" />
<PackageReference Include="Microsoft.DotNet.XUnitExtensions" Version="2.4.0-prerelease-63213-02" />
<PackageReference Include="coverlet.msbuild" Version="$(CoverletVersion)" />
<PackageReference Include="coverlet.collector" Version="$(CoverletCollectorVersion)" />
</ItemGroup>

<ItemGroup>
Expand All @@ -54,22 +54,4 @@
</ProjectReference>
</ItemGroup>

<PropertyGroup Condition="'$(Coverage)' == 'true'">
<!-- https://github.com/tonerdo/coverlet/issues/618 -->
<IncludeTestAssembly>true</IncludeTestAssembly>

<CollectCoverage>true</CollectCoverage>
<SingleHit>true</SingleHit>
<CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies> <!-- https://github.com/tonerdo/coverlet/issues/72 -->
<CoverletOutputFormat>opencover</CoverletOutputFormat>
<CoverletOutput>$(BaseOutputPath)$(PlatformConfig)\coverage\$(MSBuildProjectName).coverage</CoverletOutput>
<Include>[Microsoft.ML.*]*</Include>
<!-- Excluding for perf reasons. These classes have tests that can be run conditionally
but they need to be migrated. Excluding these classes should have very minimal effect on code coverage.
-->
<Exclude>[*]Microsoft.ML.*Contracts*,[*]Microsoft.ML.Internal.Utilities*,[*]Microsoft.ML.Data.VBuffer*</Exclude>
<ExcludeByAttribute>Obsolete,ExcludeFromCodeCoverage</ExcludeByAttribute>
<ExcludeByFile>$(RepoRoot)src\Microsoft.ML.OnnxConverter\OnnxMl.cs,$(RepoRoot)src\Microsoft.ML.TensorFlow\TensorFlow\Buffer.cs,$(RepoRoot)src\Microsoft.ML.TensorFlow\TensorFlow\Tensor.cs,$(RepoRoot)src\Microsoft.ML.TensorFlow\TensorFlow\Tensorflow.cs</ExcludeByFile>
</PropertyGroup>

</Project>
3 changes: 3 additions & 0 deletions test/Microsoft.ML.Tests/OnnxConversionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,9 @@ public void OneHotHashEncodingOnnxConversionTest()
var onnxModelPath = GetOutputPath(onnxFileName);
SaveOnnxModel(onnxModel, onnxModelPath, null);

//Free up memory for x86 test
GC.Collect();

@harishsk harishsk May 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? And how does this affect code coverage?

@frank-dong-ms-zz frank-dong-ms-zz May 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I come up with out of memory issue for x86 in local env after upgrade package "Microsoft.NET.Test.Sdk".
So I'm assuming x86 test is close to use up all 2 GB memory. We should optimize memory usage like free up memory as soon as possible but that seems like out of this PR's scope.

In reply to: 432260986 [](ancestors = 432260986)

@harishsk harishsk May 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we running code coverage on x86 builds? If not, we should remove this from this PR and address it separately.

In reply to: 432322296 [](ancestors = 432322296,432260986)

@frank-dong-ms-zz frank-dong-ms-zz May 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we only run code coverage on x64, netcore app 2.1, but we run x86 CI builds, this happens not from code coverage build but normal CI build only after upgrade Microsoft.NET.Test.Sdk.

In reply to: 432630210 [](ancestors = 432630210,432322296,432260986)

@harishsk harishsk May 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breast-cancer.txt is a 15K file. OneHotHashEncoding is just doing a bunch of computation and not allocating memory. It is very suspicious that it would run out of memory during this test. Are you able to reproduce this consistently?

In reply to: 432724322 [](ancestors = 432724322,432630210,432322296,432260986)

@frank-dong-ms-zz frank-dong-ms-zz May 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have several repro at local, haven't try on CI. Memory issue may not be at this test, what I observe is that when we start to see out of memory issue at local, we see this issue from several different tests but we always starts to see out of memory from this specific test. This is can be caused from previous code/test not releasing memory in time.

In reply to: 432725851 [](ancestors = 432725851,432724322,432630210,432322296,432260986)

@harishsk harishsk May 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it is not appropriate to add a GC.Collect here. Ideally we shouldn't be adding a GC.Collect anywhere and instead rely on predictable disposal of allocated memory. But if we have to add it, we should add it where the allocation is happening instead of somewhere else.

In reply to: 432738561 [](ancestors = 432738561,432725851,432724322,432630210,432322296,432260986)

@frank-dong-ms-zz frank-dong-ms-zz May 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try on CI without this GC.Collect, if CI can pass then we can optimize memory issue in separate PR, other wise I would like to optimize memory usage first or disable some test from running on x86 first

In reply to: 432739891 [](ancestors = 432739891,432738561,432725851,432724322,432630210,432322296,432260986)

@harishsk harishsk May 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try adding memory statistics to BaseTestClass.Cleanup so that you can get a log of the memory usage at the end of each test to try and find the offending test.

In reply to: 432761446 [](ancestors = 432761446,432739891,432738561,432725851,432724322,432630210,432322296,432260986)

@frank-dong-ms-zz frank-dong-ms-zz May 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is great idea, I will do that in a separate PR since x86 test runs fine on CI due to my test

In reply to: 432787073 [](ancestors = 432787073,432761446,432739891,432738561,432725851,432724322,432630210,432322296,432260986)


if (IsOnnxRuntimeSupported())
{
// Evaluate the saved ONNX model using the data used to train the ML.NET pipeline.
Expand Down
18 changes: 18 additions & 0 deletions test/coverlet.runsettings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="utf-8" ?>
<RunSettings>
<DataCollectionRunSettings>
<DataCollectors>
<DataCollector friendlyName="XPlat code coverage">
<Configuration>
<Format>opencover</Format>
<Exclude>[*]Microsoft.ML.*Contracts*,[*]Microsoft.ML.Internal.Utilities*,[*]Microsoft.ML.Data.VBuffer*</Exclude> <!-- [Assembly-Filter]Type-Filter -->
<Include>[Microsoft.ML.*]*</Include> <!-- [Assembly-Filter]Type-Filter -->
<ExcludeByAttribute>Obsolete,ExcludeFromCodeCoverage</ExcludeByAttribute>
<ExcludeByFile>$(RepoRoot)src\Microsoft.ML.OnnxConverter\OnnxMl.cs,$(RepoRoot)src\Microsoft.ML.TensorFlow\TensorFlow\Buffer.cs,$(RepoRoot)src\Microsoft.ML.TensorFlow\TensorFlow\Tensor.cs,$(RepoRoot)src\Microsoft.ML.TensorFlow\TensorFlow\Tensorflow.cs</ExcludeByFile> <!-- Globbing filter -->
<SingleHit>true</SingleHit>
<IncludeTestAssembly>true</IncludeTestAssembly>
</Configuration>
</DataCollector>
</DataCollectors>
</DataCollectionRunSettings>
</RunSettings>
7 changes: 6 additions & 1 deletion test/run-tests.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@

<Target Name="RunTests">
<Message Importance="High" Text="Running tests ..." />
<MSBuild Targets="VSTest"
<MSBuild Condition="'$(Coverage)' == 'true'"
Targets="VSTest"
Projects="@(Project)"
Properties="VSTestNoBuild=true;VSTestBlame=true;VSTestCollect=XPlat Code Coverage;VSTestSetting=$(MSBuildThisFileDirectory)\coverlet.runsettings" />
<MSBuild Condition="'$(Coverage)' == 'false'"
Targets="VSTest"
Projects="@(Project)"
Properties="VSTestNoBuild=true;VSTestBlame=true" />
</Target>
Expand Down