-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Helix Testing #6992
Helix Testing #6992
Conversation
…that one in Helix to attempt to bootstrap the helix test process.
…aths inside the artifact to separate out build flavors.
…wer nuget to see if it goes faster. actually build helix config for our module and run the one for our module.
Oh my gosh is it finally ready? That's amazing |
For reviewers: The most pertinent thing to review is probably |
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.
Okay so I can't honestly say I understand all the powershell that's going on here. I'm kinda trusting that
- It's mostly copy-pasta from WinUI
- It works, so I'm okay with that
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 have to take some of this on faith.
…dsOn statement. Update more TAEF versions. Change creator of Helix job to Terminal (from WinUI)
Hello @miniksa! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Dustin L. Howett * Clear the last error before calling Mb2Wc in ConvertToW (GH-7391) * Update clang-format to 10.0 (GH-7389) * Add til::static_map, a constexpr key-value store (GH-7323) James Holderness * Refactor VT control sequence identification (CC-7304) Mike Griese * Compensate for VS 16.7, part 2 (GH-7383) * Add support for iterable, nested commands (GH-6856) Michael Niksa * Helix Testing (GH-6992) * Compensate for new warnings and STL changes in VS 16.7 (GH-7319) nathpete-msft * Fix environment block creation (GH-7401) Chester Liu * Add initial support for VT DCS sequences (CC-6328) Related work items: #28791050
This does two bits: 1. correctly marks our tests as failed in xUnit, so that AzDo will pick up that the tests have failed. 2. Actually intentionally mark skipped tests as skipped in xUnit. We were doing this accidentally before. 3. Add a CI step to log test failures in a way that they can show up on GitHub Probably regressed around #6992 and #4490. ### details #### Part the first We were relying on the MUX build scripts to convert our WTT test logs to xUnit format, which AzDo then ingests. That script we used relied on some WinUI-specific logic around retrying tests. They have some logic to auto-retry failed tests. They then mark a test as "skipped" if it passed less than some threshold of times. Since we were never setting that variable, we would mark a test as "skipped" if it had _0_ passes. So, all failures showed up on AzDo as "skipped". Why didn't we notice this? Well, the `Run-Tests.ps1` script will still return `1` if _any_ tests failed. So the test job would fail if there was a failure, AzDo just wouldn't know which test it was. #### part the second Updates `ConvertWttLogToXUnitLog` in `HelixTestHelpers.cs` to understand that a test can be skipped, in addition to pass/fail. Removes all the logic for dealing with retries, cause we didn't need that. #### part the third TAEF doesn't emit error messages in a way that AzDo can immediately pick up on which tests failed. This means that Github gives us this useless error message: ![image](https://github.com/microsoft/terminal/assets/18356694/3be6de00-22e1-421c-93d4-176bd2be4cab) That's the only "error" that AzDo knows about. This PR changes that by adding a build step to manually parse the xUnit results, and log the names of any tests that failed. By logging them with a prefix of `##vso[task.logissue type=error]`, then AzDo will surface that text as an error message. GitHub can then grab that text and surface it too. ### Addenda: Why aren't we using the VsTest module as noted in #4490 (comment), the vstest module is literally 6x slower than just running TAEF directly.
Use the Helix testing orchestration framework to run our Terminal LocalTests and Console Host UIA tests.
References
Creates the following new issues:
Consumes from:
Produces for:
Related:
te.exe
directly instead of using VSTest runner #4490 - We lost the AzDO integration of our test data when I moved from the TAEF/VSTest adapter directly back to TE. Thanks to the WTTLog + Helix conversion scripts to XUnit + new upload phase, we have it back!PR Checklist
Detailed Description of the Pull Request / Additional comments
We have had two classes of tests that don't work in our usual build-machine testing environment:
The Helix testing environment solves both of these and is brought to us by our friends over in https://github.com/microsoft/microsoft-ui-xaml.
This PR takes a large portion of scripts and pipeline configuration steps from the Microsoft-UI-XAML repository and adjusts them for Terminal needs.
You can see the source of most of the files in either https://github.com/microsoft/microsoft-ui-xaml/tree/master/build/Helix or https://github.com/microsoft/microsoft-ui-xaml/tree/master/build/AzurePipelinesTemplates
Some of the modifications in the files include (but are not limited to) reasons like:
The build now runs in a few stages:
drop
artifact to make life a little easier.)We are set to run Helix tests on the Feature test policy of only x64 for now.
Additionally, because the set up of the Helix VMs takes so long, we are NOT running these in PR trigger right now as I believe we all very much value our 15ish minute PR turnaround (and the VM takes another 15 minutes to just get going for whatever reason.) For now, they will only run as a rolling build on master after PRs are merged. We should still know when there's an issue within about an hour of something merging and multiple PRs merging fast will be done on the rolling build as a batch run (not one per).
In addition to setting up the entire Helix testing pipeline for the tests that require it, I've preserved our classic way of running unit and feature tests (that don't require an elaborate environment) directly on the build machines. But with one bonus feature... They now use some of the scripts from MUX to transform their log data and report it to AzDO so it shows up beautifully in the build report. (We used to have this before I removed the MStest/VStest wrapper for performance reasons, but now we can have reporting AND performance!) See https://dev.azure.com/ms/terminal/_build/results?buildId=101654&view=ms.vss-test-web.build-test-results-tab for an example.
I explored running all of the tests on Helix but.... the Helix setup time is long and the resources are more expensive. I felt it was better to preserve the "quick signal" by continuing to run these directly on the build machine (and skipping the more expensive/slow Helix setup if they fail.) It also works well with the split between PR builds not running Helix and the rolling build running Helix. PR builds will get a good chunk of tests for a quick turn around and the rolling build will finish the more thorough job a bit more slowly.
Validation Steps Performed