Generate tests runsheet using Arcade SDK#8811
Conversation
github-actions
bot
added
the
area-engineering-systems
| WorkingDirectory="$(RepoRoot)" | ||
| IgnoreExitCode="true" | ||
| ContinueOnError="WarnAndContinue" | ||
| /> |
There was a problem hiding this comment.
This trick allows us generating a report as part of a test run:
However, I totally realise this may not stand the test of time (considering #8802) if we can't figure out how to bake this into the Arcade SDK.
|
|
||
| <PropertyGroup Condition="'$(BaseOutputPath)' == '' and '$(TestsRunningOutsideOfRepo)' == 'true' and '$(PrepareForHelix)' != 'true' "> | ||
| <BaseOutputPath >$([MSBuild]::NormalizeDirectory($([System.IO.Path]::GetTempPath()), $([System.IO.Path]::GetRandomFileName())))</BaseOutputPath> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
This is how we get the E2E project to run in the "out of repo" mode. It also allows us testing it locally with ease.
| <_CreateRunsheet Condition=" '$(_RequiresPackages)' == 'true' and '$(FullE2e)' == 'true' ">true</_CreateRunsheet> | ||
| <_CreateRunsheet Condition=" '$(_RequiresPackages)' != 'true' and '$(FullE2e)' != 'true' ">true</_CreateRunsheet> | ||
|
|
||
| <_CreateRunsheet Condition=" '$(MSBuildProjectName)' == 'Aspire.Templates.Tests' ">false</_CreateRunsheet> |
There was a problem hiding this comment.
@radical this is where the Templates are locked out atm; it should be a matter of removing this line.
RussKie
force-pushed
the
igveliko/gharunner
branch
from
bd8de15 to
f2ae79a
Compare
|
So the test failed in this PR, how do I find the failing test in the ux? |
GHA don't offer test reporting capabilities at all (unlike AzDO). There are several 3P products available, but the UX I've seen is still generally unrefined compared to the one of AzDO. The current implementation doesn't offer any visibility other than parsing the log on screen. This pull-request introduces a different mechanic for running tests (based on the Arcade SDK), and whenever there's a test failure one will have to look at the failed step. For example, here's a failed step: One can also download the detailed logs and inspect those: |
|
To clarify - the tests are still being run using the existing implementation, and the new flow can only be tested after this PR is merged. |
|
We a much better great experience with the gihub test logger, can we build something similar? |
|
The test reporter was discussed on the PR that removed it - https://github.com/dotnet/aspire/pull/8498/files#r2025259310
I'm not working on an alternative to that. |
|
Also, because of #8802 the old behavior with the test reporter is back, for now. |
Youssef1313
left a comment
Youssef1313
left a comment
There was a problem hiding this comment.
Is this urgent, or could it wait a bit to see if #8833 works so it remains built on top of MTP?
@davidfowl We can have something similar for MTP for sure. It shouldn't be hard to implement it as an MTP extension. Feel free to reach out internally to discuss :) |
|
@joperezr mentioned that there's someone under @timheuer may be working on a reporter. I also stumbled yesterday across https://github.com/test-summary/action, perhaps it could be forked and adapted? |
I think this can wait since this is more an overall improvement to the existing functionality. |
|
Hmm, https://github.com/test-summary/action could be an option yes! It works only for JUnit test format, but xunit.v3 has support for producing JUnit XML test results so that can work. And that specific one is very unrelated to the test platform being used. It just takes the XML file and does its job. So it doesn't matter if it's VSTest or MTP being used. |
|
There's a PR adding trx support --> test-summary/action#59 |
|
Here's another reporter I stumbled across --> https://github.com/bibipkins/dotnet-test-reporter |
RussKie
force-pushed
the
igveliko/gharunner
branch
from
f2ae79a to
66cbffd
Compare
|
Clean rebase to account for #8833. |
| @@ -0,0 +1,195 @@ | |||
| # Executes all the tests on all the platforms | |||
There was a problem hiding this comment.
Does this replace tests.yml+run-tests.yml? How is this being used?
There was a problem hiding this comment.
Yes, that's the plan to use this instead of the other existing workflows.
Right now, this is to be kicked off manually, since this runsheet isn't running the template tests (per your request). We can enable running this alongside with the existing workflow (to test the templates) but we'll need to remove all other tests from test.yml. Essentially, this is the follow up work.
There was a problem hiding this comment.
Could you please help me understand why we are repeating the stuff from tests.yml, and run-tests.yml here? Can they not be re-used with some relevant command line changes?
There was a problem hiding this comment.
This is supposed to replace tests.yml+run-tests.yml, however we can't do it right now until the Templates tests are also moved to the runsheet.
There was a problem hiding this comment.
Shouldn't we be able to re-use run-tests.yml as-is, instead of splitting and duplicating it here, and then debugging the new setup again? That way we keep using the existing stuff and replace the test generation stuff with the runsheets.
There was a problem hiding this comment.
The existing implementation does a lot more, and I don't want to spend time trying to selectively clean or refactor it to ensure the Template tests continue to run.
I need this change so we can verify it and continue to build on top of it.
There was a problem hiding this comment.
It creates extra work to review the changes here and in following PRs, which are partially repeating the existing working implementation without being sure that it works. I don't understand the reason for throwing out the working thing.
I'll approve here, and we can discuss when you enable this, I guess.
| </PropertyGroup> | ||
|
|
||
| <!-- Generate a test report --> | ||
| <Exec Command="pwsh -command "$(RepoRoot)eng/scripts/gha-testreport.ps1 -TestResultsFolder '$(_TestResultDirectory)' -TestSummaryOutputFolder '$(TestResultsLogDir)'"" |
There was a problem hiding this comment.
Why not simply implement stuff using MTP extensibility?
There was a problem hiding this comment.
It's an interesting idea, how would you approach it?
There was a problem hiding this comment.
@RussKie You'd implement an extension as both IDataConsumer and ITestSessionLifetimeHandler. It will be consume TestNodeUpdateMessages. The test node will have TimingProperty which the duration, and will have TestNodeStateProperty indicating test status (pass, fail, timeout, skipped, cancelled).
While consuming, you'll keep the data you need stored, then in ITestSessionLifetimeHandler.OnTestSessionFinishingAsync you can print the data to OutputDevice (and disable Arcade's redirection), or maybe writing to GITHUB_STEP_SUMMARY can be a better idea anyways?
RussKie
force-pushed
the
igveliko/gharunner
branch
from
8f2e2f9 to
d477819
Compare
4 participants
Generate a runsheet for all tests using the Arcade SDK, similar to how we generate runsheets for quarantined tests.
There are two parallel workflows:
1. generate a runsheet for all tests that must be run in "out of repo" mode and depend on nupkgs. These tests include the E2E and the Template tests, though the Template tests are currently excluded (per @radical's ask).
The first step of the flow is expensive because we need to generate all nupkgs, however, the second step of the flow is generally fast.
2. generate a runsheet for all other tests.
The first part is reasonably fast (it takes ~1min to install all toolsets); and the second part is as long as the longest test (considering we get all test projects run concurrently; subject to VM pool availability).
This implementation avoids a lot of existing test-related infra. One of the key changes, is that instead of archiving and unarchiving the E2E tests it is published to and executed from a temporary location.