-
Notifications
You must be signed in to change notification settings - Fork 325
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
[WIP]Add coverlet smoke test #2330
[WIP]Add coverlet smoke test #2330
Conversation
@nohwnd @jakubch1 I have strange failure running smoke test on CI and local with script
It works inside VS and also if I run command by hand on console, but fail with test script/CI
Is it expected, am I missing something? |
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.5.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the variables from the TestPlatform.Dependencies to keep the versions up to date. I am currently making changes to run the acceptance tests against the version that is built in the repo, not against the previous version. We will need to rebase this on top of the other change to allow this test to pass, othewise it would use a way old version of the dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll rebase when update will be in place...or do you want that I create a new prop NETTestSdkBuildVersion
and for now fix the version and you'll update with var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not necessary I would have to go and remove that variable afterwards. I am fighting with the acceptance tests to pass on the server, but hopefully it won't take more than a day more to get it to pass... 🤞 (or at least I hope because it is boring and exhausting 😁 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2340 is now in master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great rebasing!Any news on strange error #2330 (comment)?
this.InvokeDotnetTest($"--collect:\"XPlat Code Coverage\" {projectFileName} --diag:coverletcoverage.{logId}.log"); | ||
var currentDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); | ||
|
||
// Verify vstest.console.dll CollectArgumentProcessor fix codeBase for coverlet package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this comment means, could you rephrase it to be more concrete, or explain to me what it means in a comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR #2288 we removed custom code to allow coverlet lib to load in-proc collector.
That PR added some code inside CollectArgumentProcessor
where we looking for "base path" of coverlet.collector.dll
expected inside nuget package default folder, passed with VSTestTestAdapterPath
https://github.com/tonerdo/coverlet/blob/master/src/coverlet.collector/build/netstandard1.0/coverlet.collector.targets#L21
When we find the lib we'll emit a log https://github.com/microsoft/vstest/pull/2288/files#diff-6f2506b5205b023825192043bd33a256R138
This assert check that we're sure that we've updated collector setting code base with full path, otherwise without "custom coverlet code" inside ProxyExecutionManager
coverlet dll won't be resolved inside testhost.
https://github.com/microsoft/vstest/pull/2288/files#diff-3e65931fa6e723b1e2669b6ac7d6bcddL44
https://github.com/microsoft/vstest/pull/2288/files#diff-3e65931fa6e723b1e2669b6ac7d6bcddL272
@@ -228,6 +228,10 @@ function Publish-PatchedDotnet { | |||
Write-Log "Publish-PatchedDotnet: Copy VSTest task artifacts to local dotnet installation to allow `dotnet test` to run with it" | |||
$buildArtifactsPath = "$env:TP_ROOT_DIR\src\Microsoft.TestPlatform.Build\bin\$TPB_Configuration\$TPB_TargetFrameworkNS2_0\*" | |||
Copy-Item $buildArtifactsPath $dotnetTestArtifactsSdkPath -Force | |||
|
|||
Write-Log "Publish-PatchedDotnet: Copy vstest.console.dll artifact to local dotnet installation to allow `dotnet test` to run with it" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During my current work on the acceptance tests I found that this is falling short, because I am patching the task definition and the build but not the console and testhost. Would this be able to run via the InvokeVstest (or the other alike methods that run vsconsole but not dotnet test)? I tried taking the contents of the package that we ultimately insert into dotnet test
(the CLI package), and it broke the runner, and I would rather avoid copying "random" dlls into the local copy of dotnet because then we can't really be sure about what we are testing against and if it reflects the real world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot use InvokeVsTest...because the base path of coverlet directory(nuget package) is injected through msbuild prop https://github.com/tonerdo/coverlet/blob/master/src/coverlet.collector/build/netstandard1.0/coverlet.collector.targets
I should pass /testadapterpath:
with path #2278 (comment) but it depends on where we run tests and on which plat(this is a less important issue, coverlet pack at the moment has got only one tfm netstandard1.0)
Do you know a way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's figure that out later, imho finding the adapter should not be that hard, there will be some tests already doing it. But I would definitely love to get the dotnet test working with the currently built taks + console + testhost, because testing dotnet test more extensively would be nice. Maybe I was just doing something wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look if InvokeVstest
is used by other tests.
#2340 was pretty invasive and changed project guid...I don't want to lose too much time on fixing merging issue(I get some build error related to sln guids), I'll open new one with new test since this PR is low code one. |
closes #2297
cc: @AbhitejJohn @nohwnd @jakubch1