-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Eliminate duplicates in Helix TestRunner
#25663
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
Conversation
| set DOTNET_CLI_HOME=%HELIX_CORRELATION_PAYLOAD%\home | ||
|
|
||
| set PATH=%DOTNET_ROOT%;!PATH!;%HELIX_CORRELATION_PAYLOAD%\node\bin | ||
| echo Set path to: %PATH% |
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.
Have you confirmed the path issues no longer occur? There's a few queues that have spaces due to jdk paths which used to die miserably, issues might only show up if you run the full matrix
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
SETset command didn't cause that problem; we invokedotnet-install.ps1w/-noPathto avoid that - The issue here apparently was parentheses in the
%PATH%value causing script parsing problems; I chose the more common way to deal with that.
FYI…
Set path to: "C:\h\w\AE0A0984\p\sdk\x64;C:\python3.7.0\lib\site-packages\pywin32_system32;C:\python3.7.0\Scripts\;C:\python3.7.0\;C:\windows\system32;C:\windows;C:\windows\System32\Wbem;C:\windows\System32\WindowsPowerShell\v1.0\;C:\Program Files\Microsoft SQL Server\130\Tools\Binn\;C:\Users\runner\AppData\Local\Microsoft\WindowsApps;C:\h\w\AE0A0984\p\node\bin"
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.
BTW spaces are fine when using this quoted syntax too
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.
If you say so, that's how things started, but we'll find out soon enough :)
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.
But yeah I guess the issue I recall is with parens causing issues with IF parsing, not spaces so its only machines that have the "C:\Program Files (x86)";
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'd suggest running this through the full helix queue before merging regardless when mucking with runtests.cmd
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.
We might be okay since we don't use the path inside an IF anymore in this script which I think we used to do
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 syntax works fine for all the cases you want to throw at it other than nesting in ifs but I kicked off https://dev.azure.com/dnceng/public/_build/results?buildId=804251
Side note: That matrix only runs for the master branch and is completely on the floor. Hope #25650 gets it going again.
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.
RE: side note, is that a good idea to not run the matrix something like once a day in release branches, there's both risk in product platform regressions and even test only issues that cause the matrix to break will be hard to isolate when merging back to master if it never is run
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 agree w/ you @HaoK Something worth adding though not in this PR.
|
My aspnetcore-helix-matrix run failed for familiar problems. The main job hit the PDB duplicates issue from #25293 and the ARM64 job failed due to #25397. @halter73 @sebastienros @JunTaoLuo have we made any progress on either problem❔ I'll fix #25397 in this PR. FYI |
bef858c to
88ab118
Compare
|
New aspnetcore-helix-matrix run is https://dev.azure.com/dnceng/public/_build/results?buildId=804758 |
88ab118 to
c96ab0a
Compare
|
Latest aspnetcore-helix-matrix run is https://dev.azure.com/dnceng/public/_build/results?buildId=804868 |
|
@Tratcher @jkotalik @BrennanConroy we've got a lot of open
I'm not sure how many of these I hit w/ this PR on ARM64 but https://helix.dot.net/api/2019-06-17/jobs/a11e919b-b39a-4636-b962-50da99624b7c/workitems/dotnet-watch.Tests--net5.0/console doesn't look good. Should most of the |
|
Separately, are we seeing |
|
@HaoK in any case, the jobs were successfully submitted to all Helix agents in the most recent aspnetcore-helix-matrix build. Problems above are unrelated to my changes. |
@pranavkm is a better person to answer that. |
- do not add source that's already in NuGet.config - configuration file now part of Helix content - do not set env variable set in `runtests.*` nit: simplify `%Path% setting in runtests.cmd - always quote the value
- add `--no-restore` to `dotnet run` command after `dotnet restore`
- #25397 - was restoring for the wrong architecture nit: add `--no-restore` to Helix project build steps - already restored
- don't pollute environment on Helix agents
49eeb10 to
d0b15c1
Compare
runtests.*nit: simplify `%Path% setting in runtests.cmd