Error on duplicate test output dirs #16521

sfoslund · 2021-03-25T16:15:01Z

dotnet-issue-labeler · 2021-03-25T16:15:11Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

joeloff · 2021-03-25T23:24:17Z

src/Tests/Microsoft.NET.TestFramework/TestAssetsManager.cs

@@ -128,14 +130,20 @@ public string GetAndValidateTestProjectDirectory(string testProjectName, string
                using (var sha256 = SHA256.Create())
                {
                    var hash = sha256.ComputeHash(Encoding.UTF8.GetBytes(directoryName.ToString()));
+                    var rand = Guid.NewGuid().ToString("N").Substring(0, 5);


Path.GetRandomFileName perhaps, or do you need a shorter random string?

dsplaisted

I would prefer not to use random paths. If we have to, I'd prefer to do it only in CI runs so that you have a predictable path locally, and so that you don't have to keep cleaning up your test folder to stop it from growing without bounds.

Alternatively, could we have a mode that makes it an error if the test folder already exists when we try to create it? Then that would help us find all of the tests that have overlapping test folders.

wli3 · 2021-03-26T00:03:58Z

Per #16425 (comment). We've been trying to get a stable path without conflict for years but we still cannot get it right. And I think it is good enough that the test can print out the temp directory,

sfoslund · 2021-03-26T16:31:45Z

We've been trying to get a stable path without conflict for years but we still cannot get it right. And I think it is good enough that the test can print out the temp directory

So you'd prefer we make it random and print out the directory name? Or print the name that's already being generated?

dsplaisted · 2021-03-26T17:17:24Z

Per #16425 (comment). We've been trying to get a stable path without conflict for years but we still cannot get it right. And I think it is good enough that the test can print out the temp directory,

We've just been playing whack-a-mole, when we have a test failing that looks like it might be due to this we look for another test that would use the same folder. We should be able to deterministically find all the conflicts by adding logic to error out if the test folder already exists when we try to create it, and running that with an empty test folder.

danmoseley · 2021-03-26T18:37:34Z

Note there is a middle path, which is deterministic + a random suffix. Eg., c:\foo\bar......\mytestmethod.ASD2435SDF ... that avoids collisions, but still leaves a hint as to which test.

I have no preference.

sfoslund · 2021-03-29T19:36:08Z

We should be able to deterministically find all the conflicts by adding logic to error out if the test folder already exists when we try to create it, and running that with an empty test folder.

I did a test run with this error and there are quite a lot of individual cases we'll have to handle with this approach. Do we want to allow theories to use the same test folders?

dsplaisted · 2021-03-29T20:06:01Z

We should be able to deterministically find all the conflicts by adding logic to error out if the test folder already exists when we try to create it, and running that with an empty test folder.

I did a test run with this error and there are quite a lot of individual cases we'll have to handle with this approach. Do we want to allow theories to use the same test folders?

Yes, ideally each instance of a theory would use a separate test folder. The identifier parameter to CreateTestProject or similar methods can be used for this.

It's possible that a theory not using different test folders for each instance is less likely to cause flaky failures than an entirely separate test case, as I believe unrelated test cases can be run concurrently while instances of theories are run sequentially.

dsplaisted

Thanks a bunch for fixing all of these!

dsplaisted · 2021-04-01T02:58:54Z

src/Tests/Microsoft.NET.TestFramework/TestAssetsManager.cs

+            if (Directory.Exists(directoryPath))
+            {
+                throw new Exception($"Test dir {directoryPath} already exists");
+            }


By default, we should only error out when running in CI. Otherwise rerunning tests locally will fail by default unless you delete the test directory manually each time.

Do we have a flag for if the tests are running in CI already?

I think there is an MSBuild property or environment variable. Try downloading a binlog from a CI run. Also when the tests are run in helix there may be something that tells us about that. @wli3

I think we can make it an error on default. And add an extra "set environment" and a new env in .\artifacts\sdk-build-env.bat and .\artifacts\sdk-build-env.sh

we need to run .\artifacts\sdk-build-env.bat to build sdk anyway

we need to run .\artifacts\sdk-build-env.bat to build sdk anyway

I basically always run it but a lot of the time it's not necessary. I imagine most people that try to use our repo will just open the .sln in Visual Studio and they'll want the tests to work.

Agreed, I'm went ahead and conditioned it based on the ContinuousIntegrationBuild property.

will just open the .sln in Visual Studio and they'll want the tests to work.

nothing will work by just open .sln in visual studio

sfoslund · 2021-04-05T17:57:59Z

@dsplaisted @wli3 is this good to go?

Port 'Error on duplicate test output dirs #16521'

Reference: #16521

sfoslund force-pushed the TestOutput branch 2 times, most recently from 7308958 to 3d30dcb Compare March 25, 2021 21:31

sfoslund requested review from wli3, dsplaisted and joeloff March 25, 2021 23:04

joeloff reviewed Mar 25, 2021

View reviewed changes

dsplaisted reviewed Mar 25, 2021

View reviewed changes

sfoslund force-pushed the TestOutput branch from 3d30dcb to f6f4ba1 Compare March 30, 2021 22:56

sfoslund changed the title ~~Adding random string to test directory generation~~ Error on duplicate test output dirs Mar 30, 2021

Error on duplicate test output dirs

696d32f

sfoslund force-pushed the TestOutput branch from f6f4ba1 to 696d32f Compare March 31, 2021 16:00

sfoslund requested a review from dsplaisted March 31, 2021 19:44

dsplaisted reviewed Apr 1, 2021

View reviewed changes

Only error for duplicate output dirs on CI builds

685fc8e

dsplaisted approved these changes Apr 5, 2021

View reviewed changes

sfoslund merged commit e4655fd into dotnet:main Apr 5, 2021

sfoslund deleted the TestOutput branch April 5, 2021 20:01

sfoslund added a commit to sfoslund/sdk that referenced this pull request Apr 7, 2021

Port Error on duplicate test output dirs dotnet#16521

d9dd027

sfoslund added a commit to sfoslund/sdk that referenced this pull request Apr 7, 2021

Port Error on duplicate test output dirs dotnet#16521

f28a004

sfoslund added a commit that referenced this pull request Apr 12, 2021

Merge pull request #16778 from sfoslund/TestOutput502

1c810cf

Port 'Error on duplicate test output dirs #16521'

TanayParikh added a commit that referenced this pull request Nov 10, 2021

Try fix test dir already exists

d8e295e

Reference: #16521

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error on duplicate test output dirs #16521

Error on duplicate test output dirs #16521

sfoslund commented Mar 25, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Mar 25, 2021

joeloff Mar 25, 2021

dsplaisted left a comment

wli3 commented Mar 26, 2021

sfoslund commented Mar 26, 2021

dsplaisted commented Mar 26, 2021

danmoseley commented Mar 26, 2021

sfoslund commented Mar 29, 2021

dsplaisted commented Mar 29, 2021

dsplaisted left a comment

dsplaisted Apr 1, 2021

sfoslund Apr 1, 2021

dsplaisted Apr 2, 2021

wli3 Apr 2, 2021

wli3 Apr 2, 2021

dsplaisted Apr 2, 2021

sfoslund Apr 2, 2021

wli3 Apr 3, 2021 •

edited

Loading

sfoslund commented Apr 5, 2021

Error on duplicate test output dirs #16521

Error on duplicate test output dirs #16521

Conversation

sfoslund commented Mar 25, 2021 • edited Loading

dotnet-issue-labeler bot commented Mar 25, 2021

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

wli3 commented Mar 26, 2021

sfoslund commented Mar 26, 2021

dsplaisted commented Mar 26, 2021

danmoseley commented Mar 26, 2021

sfoslund commented Mar 29, 2021

dsplaisted commented Mar 29, 2021

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

sfoslund commented Apr 5, 2021

sfoslund commented Mar 25, 2021 •

edited

Loading

wli3 Apr 3, 2021 •

edited

Loading