-
Notifications
You must be signed in to change notification settings - Fork 331
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
Avoid guid at the end of outputDirectory #2378
Comments
I don't know the reasons for the guid but I would rather avoid yet another option. I see the coverage file described in the TRX output, but even thought there is a href to it, it does not contain the guid. I also don't see the guid anywhere else in the repo. Imho the best course of action is to provide a way that will allow the test result to be programatically processed. When you run with CC in azure pipeline, do you see the coverage file automatically uploaded? maybe they have this already figured out. |
Admittedly the trx, and xml in general is not the easiest to process, but it's a start, and it is generic format that we don't have to re-invent. |
Looking at publish result task https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/test/publish-test-results?view=azure-devops&tabs=yaml seem that a glob filter is used - task: PublishTestResults@2
inputs:
testRunner: VSTest
testResultsFiles: '**/*.trx'
failTaskOnFailedTests: true |
I'm ok with file format, but non deterministic path add some work to do in some scenario, tipical "find right file and move to". |
I meant it more in the direction of returning a simple object that can be programatically processed to get to the info, instead of forcing the naming conventions to be deterministic. For example in Pester I simply return a result object that the caller can process. Here we could do something similar. Say you'd want to upload your CC file: $result = dotnet test --collect="Code Coverage" ....params.... --json-result
($result | ConvertFrom-Json).Attachements | where Type -eq "Code Coverage" | Invoke-RestMethod -Method PUSH This would be the end product where we would have to come up with the result etc... So a more achievable goal would be to hookup the trx file and read the path to the CC file from there if it was correct. |
If I don't use powershell what does |
Yes, it was just an exam
…________________________________
Od: Marco Rossignoli <[email protected]>
Odesláno: Wednesday, April 1, 2020 8:05:48 PM
Komu: microsoft/vstest <[email protected]>
Kopie: Jakub Jareš <[email protected]>; Comment <[email protected]>
Předmět: Re: [microsoft/vstest] Avoid guid at the end of outputDirectory (#2378)
$result = dotnet test --collect="Code Coverage" ....params.... --json-result
Are you forcing powershell usage?If I don't use powershell what does --json-result produce?Is it a json on standard output?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#2378 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABLYLYIFL7VICTTBJS7R7ILRKN67ZANCNFSM4LVPYFMA>.
|
Yes json, it was just an example to show a concept. I am not forcing powershell on anyone.
…________________________________
Od: Marco Rossignoli <[email protected]>
Odesláno: Wednesday, April 1, 2020 8:05:48 PM
Komu: microsoft/vstest <[email protected]>
Kopie: Jakub Jareš <[email protected]>; Comment <[email protected]>
Předmět: Re: [microsoft/vstest] Avoid guid at the end of outputDirectory (#2378)
$result = dotnet test --collect="Code Coverage" ....params.... --json-result
Are you forcing powershell usage?If I don't use powershell what does --json-result produce?Is it a json on standard output?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#2378 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABLYLYIFL7VICTTBJS7R7ILRKN67ZANCNFSM4LVPYFMA>.
|
What if we have more than one collector https://github.com/Microsoft/vstest-docs/blob/master/docs/analyze.md#enabledisable-a-datacollector? |
Just stumbled upon this.
I disagree. VSTest should respect the specified
I would expect the following behavior:
In this example you can even see that coverage is explicitly listed in the comment (which I took from the docs). Specifying the log file name seems to have worked in the past with the pre-coverlet code coverage collector: #1957. |
@ViktorHofer I think it's a dup of #2334 |
You are right :) |
FWIW I currently need to clean the TestResults directory manually before VSTest is invoked to make sure that ReportGenerator picks the right coverage results file later. Ideally we would not need to do this. |
The directory where the coverage results will be published is required to exist. Otherwise it throws:
That's also bad. |
I think I see the purpose for the dynamic folder that is appended-- if you run it at the root level against a solution with multiple test projects, each project is in its own dynamic GUID folder. A better solution would be to at least use the project name in this scenario. These names are predictable and should be unique: ...unless there's some weird nesting in folders that would allow duplicate project names. That sounds like an edge case that the user would have to fix, though, and in that case you could use GUID folders per duplicated project or something. |
Spent the evening doing tests here and there trying to understand why I was getting random folders. Glad that I finally found this, but sad this is not implemented. This is definitively not CI/CL friendly. |
This is also not IDE friendly, as there is no deterministic way to find the most recent result folder. Coverage highlighting tools (VSCode Coverage Gutters, in my case) can't be effectively configured to work with this behavior. |
dotnet test standard output provides the generated folder name "Starting test execution, please wait...\r\nLogging Vstest Diagnostics in file: C:\Users\tonyh\Source\Repos\DataCollectorXUnit\XUnitTestProject1\bin\Debug\netcoreapp3.1\fine-code-coverage\coverage-tool-output\diagnostics.log\r\nA total of 1 test files matched the specified pattern.\r\n\r\n The diagnostic log also has the generated folder name TpTrace Verbose: 0 : 4712, 10, 2021/02/21, 14:58:38.317, 1073211811845, vstest.console.dll, DataCollectionRequestSender.SendAfterTestRunStartAndGetResult : Received message: (DataCollection.AfterTestRunEndResult) -> [ Agree that should avoid the guid though. |
Is there any solution found for this issue? |
@psvaiter Here is my solution. I call this executable before using the coverage file. The tool search for the coverage file and copy at the specified file path. `using System; namespace CopyCoverageFile
}` |
Hoping this may help a few people out there, I use the following snippet of PowerShell in my development scripts to extract the unit test results paths and feed them to report generator. $TestOutput = dotnet test --collect "XPlat Code Coverage"
$TestReports = $TestOutput | Select-String coverage.cobertura.xml | ForEach-Object { $_.Line.Trim() } | Join-String -Separator ';'
dotnet reportgenerator "-reports:$TestReports" "-targetdir:./CoverageReport" "-reporttype:Html" |
@ArchieCoder I tried to use MSBuild integration and that worked as expected. Besides that I could use the Fine Code Coverage extension for Visual Studio that seems to depend on coverlet.msbuild package. Here's the command I run:
This way I stopped using the vstest. |
I am right there with you. I'm not sure what genius thought an artifact directory's name should be non-deterministic, but it's completely ludicrous to me. Nobody has, from my reading, presented a single good argument as to why you'd want a GUID for this. I could see the argument for a timestamp, but honestly, if somebody wants their artifacts in a GUID-named folder, make them do it themselves as its a lot easier to go deterministic->random than vice-versa. I'm not sure why those who want consistency are the ones being punished. |
Related issue #2334 (comment) |
@MarcoRossignoli @nohwnd is someone actively working on this? It's still causing unreasonable pain and the issue is upvoted quite a lot. Also, the milestone seems to be incorrect as 17.1 already shipped. |
@ViktorHofer We are not working on this. I will bring it up at tomorrow's standup. |
Any progress? Everything that needed to be said was said in this topic. User should have an option to set static output directory and given the feedback, pretty sure this should be the default behavior. Remember that you can always work incrementally on the final solution. First integrate the proposal found here (or anything similar) and then follow up with something neater if an additional option is not something you guys are aiming for. If this issue is gone, please ignore. Using VS 2022 17.3.1 and the problem is still reproducing with |
Provided (another?) example of run script workarounding this issue. Tested on my Win10 with msys2 and Debian9. |
I believe the pseudo code for the solution that Microsoft could do is the following code. I have an hard time thinking it would be more complex than this. var fixedDirectory = String.Empty; if (args.Contains(“/fixed_directory”)) // Regular code if (!string.IsNullOrEmpty(fixedDirectory)) |
Well, I've stumbled into this & the related issues as I'm encountering the same problems. I kept seeing duplicate files in the differing folder names (guid -vs- generated name). A bit saddened to see this has been an ongoing issue for what looks to be 2+ years now. So far, I've seen the suggestion of parsing the TRX file to get the associated coverage file may be an avenue (but definitely not preferred) and then it seems like the other option is that if we know the folder-naming convention for the UserName + HostName prefix variant we could possibly just ignore/purge those as known dupes. What I wanted to report & share (and maybe get an answer for) is that I have observed some very odd behavior along these lines; In my case I have 6 test projects and I use the --results-directory argument to point to my desired output path at the repo root (TestResults) along with the --logger "trx;LogFileName=MyProject.trx" argument and that does yield the correct/expected behavior. But what I've noticed in regard to the code coverage result files is that the GUID-based directory is always present, and the file is always there, -but- I have seen the other User/Host name prefixed folders NOT get emitted reliably. And it's random it seems as I've not identified any pattern as to when it fails to emit that variant. So, add me as +1 anxiously awaiting a good solution to this problem space. I'd very much prefer to have more control over both the location where the coverage file is output and the name of it without the need to detect & handle dupes. I realize this may require an effort by both the msft/vstest team and the coverlet team as they're just build atop the provided plumbing. |
After investigating if parsing the TRX file was something I wanted to pursue using, it looks like that is going to prove to be inadequate as well. In my case the path being mentioned in the 'attachments' is using the host-name - not the GUID-based folders. So, it's still ambiguous and as I was sharing in my case, I've seen those UserName+HostName folders not getting created reliably. Seems like I'm basically stuck with the 1 option of just ignoring/purging the folders that follow the UserName+HostName convention. One question I do have: One of my key concerns is that when you process/analyze the code coverage files there is the notion of a 'hit count' and these dupes would be incrementing that count when they are merged - yielding misleading metrics potentially. Is that a correct assumption? Here's an example of the contents from my TRX file.
|
Needed as a workaround for microsoft/vstest#2378 , because random GUID folder names will not play nicely with CI pipelines.
Sharing the below workaround. Note the single - task: PublishCodeCoverageResults@1
displayName: Publish code coverage from $(Agent.TempDirectory)/*/*.cobertura.xml
inputs:
codeCoverageTool: Cobertura
summaryFileLocation: $(Agent.TempDirectory)/*/*.cobertura.xml |
I was absolutely struggling with this and I'm so glad that you pasted this in here. Thanks for an easy fix! |
Instead of using a PowerShell snippet to do the operation "find the file in a random directory and copy it to someplace known", one can simply use a tool that can copy with "wildcard source paths". However, neither So, make sure you give a known output file name and use scp:
Note, how the Code Coverage collector doesn't even respect the CoverageFileName since it simply changes the file extension... Way to go! |
@Evangelink: What did mentioning this problem in your standup on 2022-08-04 bring as a result? |
Hi @jachstet-sea, We discussed these issues and we cannot do much because any change will have consequences on a lot of people. We will keep this issue open in case we can plan some breaking changes in some version. |
Thanks for the update :) |
Sorry we cannot offer anything better at the moment |
@Evangelink If adding a parameter would cause the command line tool to break (which I doubt really), did your team consider fork this project and remove the guid creation? |
Too bad this has not been addressed. In an attempt to workaround this issue and get deterministic output paths I wrote a an MSBuild target that moves files and deletes the directory with the guid. And of course I shot myself in the feet, deleted the wrong directory and now I'm running recovery software. I'm crossing fingers that I can recover my git repository where I had a lot of local branches (and stashes). |
So that we don't end up nuking the whole git repository! It was my misunderstanding that targets setup with `AfterTargets` would not run if the target specified was skipped because its condition would evaluate to false. When trying to add multitargeting, the `MoveCoverletReport` target ran anyway and the `CoverletReport` item evaluated to empty which in turn made `CoverletReportParentDirectory` evaluate to `..`, i.e the root of the git repository which was then deleted by the `RemoveDir` task. Two safeties have been added: 1. Add the same `Condition="$(TargetFramework) != ''"` on all targets running after `GenerateHtmlCoverageReport`. 1. Add `Condition="$(CoverletReport) != ''"` when defining the `CoverletReportParentDirectory` item. This incident would never have happened in the first place if: 1. I could have used `$([System.IO.Directory]::GetParent($(CoverletReport)))` instead of `$([System.IO.Path]::Combine($(CoverletReport),'..'))` because the former would have crashed when `$(CoverletReport)` evaluates to empty, the latter just silently picked the wrong parent directory. But I did not use the former because of a [Rider bug][1]. 2. VS Test would not [create test results in non deterministic paths][2]. The `MoveCoverletReport` target would not have been needed at all. [1]: https://youtrack.jetbrains.com/issue/RIDER-92994 [2]: microsoft/vstest#2378
@Evangelink did you consider adding a parameter that allows deterministic output paths, i.e. |
After a long amount of searching around, what I had to do to solve this problem (alluded to in previous comments) is to add the following to my <PackageReference Include="coverlet.msbuild" Version="6.0.0"/> And then I had to switch from the following command:
To:
|
As long as you don't mind this hanging over your head: If you use the coverlet.msbuild way of solving this problem, be aware that you may run into this and have to revert to using coverlet.collector and have to deal with the non-deterministic nonsense in the end anyway (like I did). |
You can also use dotnet-coverage tool to resolve this issue:
|
Check how to use |
This is a new feature and won't be implemented, we are focusing on adding new features to Testing.Platform instead. https://aka.ms/testingplatform |
On Coverlet we have some users complain about the dynamic folder name generated for reports file.
I did some check on code and seem that guid is "mandatory" and not removable.
https://github.com/microsoft/vstest/blob/master/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs#L101-L125
Could be useful add a parameter, maybe in runsetting like
<DeterministicOutputdirectory>true/false</DeterministicOutputdirectory>
and in that case avoid to concat guid.ToString().Dynamic folder makes CI more complex.
Issues refs coverlet-coverage/coverlet#500 coverlet-coverage/coverlet#767
cc: @DaleyKD
The text was updated successfully, but these errors were encountered: