Conversation
Test assemblies run as concurrent processes sharing the machine-wide temp folder, so counting MSBuild_*.txt files makes the invariant fail when an unrelated process deletes a baseline file, and lets an added file cancel out a removed one so a real node crash goes unreported. Compare the set of file names instead, and report the offending file names, PIDs and timestamps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GetMSBuildLogFiles already de-dupes with StringComparer.InvariantCultureIgnoreCase, while Except used the default ordinal comparer, so the two disagreed. Windows paths are case-insensitive, so if the two Directory.GetFiles calls ever returned different casing for the same file, that file would look new and fail the test - the same class of false positive this change set removes. It cannot mask a crash, because a dump name carries a PID and a GUID and so cannot collide with a baseline file by case alone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
File.ReadAllText ran unguarded between the glob and the delete. Because the temp folder is shared, another test process could delete the file in that window, throwing FileNotFoundException out of AssertInvariant and failing the test with a stack trace instead of a clean assert - the same cross-process false positive this change set removes. DeleteNoThrow already swallows IO errors, so only the read was exposed. A file that has vanished is skipped, since the process that deleted it is the one that will report it. A file that is present but unreadable is still reported, because it cannot be recognised as benign and must not be silently dropped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happens today
Test assemblies run in parallel as separate processes.
BuildFailureLogInvariantcountsMSBuild_*.txtfiles across several temp locations at the start of every test and asserts the count is unchanged at the end.Counting is fragile: any file that appears or disappears between the two scans changes the result, whichever process caused it.
Expected: 2, Actual: 1.The fix
Compare file identities instead of counts. Record the file names at test start, and at the end look only at names that were not there before.
A deleted file was on the original list, so it can never appear as a new name — deletions can no longer fail the test. A crash dump is always a new name, so it can no longer be cancelled out.
Both existing filters (clean node shutdown, communication traces) are unchanged, so abnormal shutdowns still fail as they do today. The failure message now names the files and includes the PID and timestamps.
Two follow-ons in the same area
Case-insensitive comparison.
GetMSBuildLogFilesalready de-dupes withInvariantCultureIgnoreCase, whileExceptused the ordinal default, so the two disagreed. These paths are case-insensitive on Windows, so differing casing between the two directory scans would make a file look new and fail the test. This cannot mask a crash: a dump name carries a PID and a GUID, so it cannot collide with a baseline file by case alone.Tolerate a file being deleted before it is read.
File.ReadAllTextran unguarded between the glob and the delete. If the file is removed in that window,ReadAllTextthrowsFileNotFoundExceptionout ofAssertInvariant, failing the test with a stack trace rather than a clean assert — the same fragility as above, with an uglier failure mode.DeleteNoThrowalready swallowed IO errors, so only the read was exposed. A file that has vanished is skipped, because whichever process deleted it read it first and will report it; a file that is present but unreadable is still reported, because it cannot be recognised as benign and must not be silently dropped.If a dump is observed and deleted by a process other than the one that produced it, the failure surfaces there rather than on the test that caused it. Nothing here changes that. The named-file message, with the PID and the timestamps, is what makes that diagnosable the next time it fires.