Skip to content
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

Fix PR inconsistent test failure #8094

Merged
merged 3 commits into from
Jun 11, 2024
Merged

Conversation

MiYanni
Copy link
Member

@MiYanni MiYanni commented Jun 11, 2024

Summary

After looking over many PRs recently, I noticed that they inconsistently fail. However, no tests are failing (all tests pass). If you look over the test log, you'll see an exception at the very bottom:

[Test Class Cleanup Failure (Microsoft.TemplateEngine.Edge.UnitTests.TemplatePackageManagerTests)] System.UnauthorizedAccessException
      System.UnauthorizedAccessException : Access to the path 'C:\Users\cloudtest\AppData\Local\Temp\TemplateEngine.Tests\616c3c66-71d5-49cb-b691-65f94d387d3d\.templateengine' is denied.
      Stack Trace:
           at System.IO.Directory.DeleteHelper(String fullPath, String userPath, Boolean recursive, Boolean throwOnTopLevelDirectoryNotFound, WIN32_FIND_DATA& data)
           at System.IO.Directory.Delete(String fullPath, String userPath, Boolean recursive, Boolean checkHost)
               /_/test/Microsoft.TemplateEngine.TestHelper/EnvironmentSettingsHelper.cs(84,0): at Microsoft.TemplateEngine.TestHelper.EnvironmentSettingsHelper.<>c.<Dispose>b__5_0(String f)
           at System.Collections.Generic.List`1.ForEach(Action`1 action)
               /_/test/Microsoft.TemplateEngine.TestHelper/EnvironmentSettingsHelper.cs(73,0): at Microsoft.TemplateEngine.TestHelper.EnvironmentSettingsHelper.Dispose()

Looking into this, there was a "hack" where if the temporary test folder could not be deleted, it slept for 2 seconds and then tried again. This second attempt is what was failing. This only seems to happen on the last test execution in the TemplatePackageManagerTests. My theory is the test framework is shutting down and, somehow, this Dispose runs after the point where it can still access this folder. So, it just fails.

Either way, since these folders are created in the Temp machine folder, it really doesn't matter if we cannot clean up the last test folder. I'd rather the PR runs not randomly fail. I've also made the catch specifically catch this UnauthorizedAccessException instead of everything. Other exceptions are not expected.

@MiYanni MiYanni requested a review from a team June 11, 2024 18:27
@MiYanni MiYanni requested a review from a team as a code owner June 11, 2024 18:27
@ViktorHofer
Copy link
Member

Fixes #8072

@ViktorHofer
Copy link
Member

Hmm according to the test failures in this PR the change didn't help.

@MiYanni
Copy link
Member Author

MiYanni commented Jun 11, 2024

@ViktorHofer

Hmm according to the test failures in this PR the change didn't help.

It did help! However, it unveiled the actual reason this hack was there:

[Test Class Cleanup Failure (Microsoft.TemplateEngine.Edge.UnitTests.LocalizationTests)] System.IO.IOException
      System.IO.IOException : The directory is not empty.
      Stack Trace:
           at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
           at System.IO.Directory.DeleteHelper(String fullPath, String userPath, Boolean recursive, Boolean throwOnTopLevelDirectoryNotFound, WIN32_FIND_DATA& data)
           at System.IO.Directory.Delete(String fullPath, String userPath, Boolean recursive, Boolean checkHost)
        /_/test/Microsoft.TemplateEngine.TestHelper/EnvironmentSettingsHelper.cs(79,0): at Microsoft.TemplateEngine.TestHelper.EnvironmentSettingsHelper.<>c.<Dispose>b__5_0(String f)
           at System.Collections.Generic.List`1.ForEach(Action`1 action)
        /_/test/Microsoft.TemplateEngine.TestHelper/EnvironmentSettingsHelper.cs(73,0): at Microsoft.TemplateEngine.TestHelper.EnvironmentSettingsHelper.Dispose()

Doing some searching, I found the exact place this hack was copied from: https://stackoverflow.com/a/47398010/294804

//delete the directory and it's contents if the directory exists
if (Directory.Exists(dirPath)) {
    try {
        Directory.Delete(dirPath, recursive: true);                //throws if directory doesn't exist.
    } catch {
        //HACK because the recursive delete will throw with an "Directory is not empty." 
        //exception after it deletes all the contents of the diretory if the directory
        //is open in the left nav of Windows's explorer tree.  This appears to be a caching
        //or queuing latency issue.  Waiting 2 secs for the recursive delete of the directory's
        //contents to take effect solved the issue for me.  Hate it I do, but it was the only
        //way I found to work around the issue.
        Thread.Sleep(2000);     //wait 2 seconds
        Directory.Delete(dirPath, recursive: true);
    }
}

So, the hack was trying to avoid this recursive deletion issue. Let me see if I can find a better solution and update the PR.

@MiYanni
Copy link
Member Author

MiYanni commented Jun 11, 2024

After looking over this thread, I've determined the only 2 exception types I need to care about are UnauthorizedAccessException and IOException. So, catching both of these should work just fine.
https://stackoverflow.com/questions/329355/cannot-delete-directory-with-directory-deletepath-true

@MiYanni MiYanni enabled auto-merge June 11, 2024 21:48
@MiYanni MiYanni merged commit 020515a into dotnet:main Jun 11, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants