One TempDirectory For all performance tests #11203
Conversation
| var path = Path.Combine(workingDir, "temp"); | ||
| Directory.CreateDirectory(path); | ||
| return path; | ||
| var tempDirectory = Path.Combine(Environment.ExpandEnvironmentVariables("%SYSTEMDRIVE%")+@"\", "PerfTemp"); |
There was a problem hiding this comment.
Can we get away with something under %USERPROFILE%?
There was a problem hiding this comment.
+"\" shouldn't be necessary with Path.Combine.
There was a problem hiding this comment.
%USERPROFILE%has the same issue of rooting our test solution deep inside and it will result in Path names longer than 265 characters limit error again. Thats why I wanted the minimal root path. Also our previous test infra had test solution in C:\Roslyn\
There was a problem hiding this comment.
Path.Combine works only for joining Directories. We need to explicitly mention the \. (Tested it)
There was a problem hiding this comment.
Re: %USERPROFILE% vs %SYSTEMDRIVE%, ok.
Re: @"\", I see...%SYSTEMDRIVE% is just "C:". It might be clearer to just do:
Environment.ExpandEnvironmentVariables("%SYSTEMDRIVE%") + "\PerfTemp"
Path.Combine isn't really buying us any additional safety, because we're assuming %SYSTEMDRIVE% never ends in a path separator, and just glancing at this code, it looks like a bug to be using Path.Combine and an explicit directory separator (the whole point of Path.Combine being to avoid assumptions about directory separators).
|
LGTM, but I'm curious to know if we ever clean up the temp folder anywhere. |
There are 2 reasons. 1. We are running into errors trying to unzip test solutions in the temp folder which is rooted deep inside the binaries folder 2. When trying to run the tests again, we need to delete the temp directories of each directory separately because we could pick up csx files from the test files. We could solve the 2nd issue by cleaning up after the run. But the 1st issue still stands
Since we dont run the iterations as a scenario, we dont need to number the scenarios
basoundr
force-pushed
the
OneTempDirectory
branch
from
caa03b9
to
2563c23
Compare
|
Is this also implemented in the |
|
@AdamSpeight2008 It has probably flowed into the Future branch by now. If you wanted to check, you could use git to see if the commit |
|
@TyOverby |
|
I'm pretty sure we don't have any VB perf tests in the new system yet. Are you sure you're looking at the right code? |
|
I'm using the cmd |
|
That is unrelated to this PR. I've renamed the issue to make this more clear. |
TyOverby
changed the title
There are 2 reasons.
We could solve the 2nd issue by cleaning up after the run. But the 1st issue still stands
Tagging @TyOverby @KevinH-MS @rchande for review