-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Replace RunTests scripts with .NET app #20337
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
environmentVariables: environmentVariables); | ||
|
||
await ProcessUtil.RunAsync($"{dotnetRoot}/dotnet", | ||
$"tool install dotnet-ef --global --version {efVersion}", |
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.
Global?
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 set the DOTNET_CLI_HOME
environment variable so it isn't "global". But maybe I should move some more of that logic out of the scripts
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.
Yes
$"vstest {target} -lt", | ||
environmentVariables: environmentVariables); | ||
|
||
if (discoveryResult.StandardOutput.Contains("Exception thrown")) |
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.
Why would this happen?
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.
Is it ever in stderr?
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'm not familiar with the specifics of this
#8107
Looks like I've got all tests running correctly now. So just need to do cleanup. |
I was invited to review this but have no context. What are the aims of this PR? |
Just figured you might be interested.
Replacing our scripts with .NET code. We know how to write in .NET and it consolidates our logic to one place instead of multiple. |
Ship it |
The overall approach is not a big deal to me either way. I am concerned about requiring that Helix agents build the new RunTests project. That will likely slow down every run enough to matter cumulatively (we do lots 'o runs). Suggest building then including the results in the job payloads.
nit: Not sure how valid the first point is given everyone needs to be familiar w/ MSBuild and scripting to do ops-on-call work. But, I agree maintaining |
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.
Nice, this is way better than maintaining cmd and sh versions!
@BrennanConroy unless you've confirmed the time spent on Helix agents hasn't worsened, please file a follow-up issue to move the RunTests.csproj build into the main job. |
It takes a couple seconds to build... this is definitely not an area we need to care about to improve PR/rolling builds... |
Well I guess its probably a little cleaner just to build the runtests thing as part of the normal builds, and just include the dll into helix content and run that dll in the scripts, since that would let us write unit tests against RunTests too, which probably is a good thing since its gotten pretty complicated now :/ |
Removing all our scripting logic from .cmd and .sh files for running tests on Helix and normalizing it into .NET code.