[review] [fix] Fix TRX LogFileName collision when testing multiple assemblies in a solution#16074
Open
nohwnd wants to merge 1 commit into
Conversation
Replace manual assembly path quoting with the established BuildMultipleAssemblyPath helper method for clarity and consistency with other tests in the codebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nohwnd
commented
May 29, 2026
Member
Author
nohwnd
left a comment
There was a problem hiding this comment.
🧠 Expert Review — PR #16074
Scope: test/Microsoft.TestPlatform.Acceptance.IntegrationTests/LoggerTests.cs — Acceptance Test Coverage Design
Analysis
This is a focused cleanup in a single acceptance test. The change replaces a manually constructed, hand-quoted assembly path string:
// Before — fragile manual quoting
var assembly1 = GetAssetFullPath("SimpleTestProject.dll");
var assembly2 = GetAssetFullPath("SimpleTestProject2.dll");
var assemblyPaths = $"{assembly1}\" \"{assembly2}";with the established BuildMultipleAssemblyPath helper:
// After — uses AddDoubleQuote() consistently with every other multi-assembly test
var assemblyPaths = BuildMultipleAssemblyPath("SimpleTestProject.dll", "SimpleTestProject2.dll");Correctness: ✅ The helper (string.Join(" ", GetTestDlls(assetNames).Select(a => a.AddDoubleQuote()))) produces exactly the same format used in all other multi-assembly acceptance tests throughout the suite. The old manual approach was the outlier.
Dimension checks (Acceptance Test Coverage Design):
- ✅ No test coverage reduction — same assemblies, same test scenario
- ✅ Consistent with the established pattern across the test suite
- ✅ No concerns with cross-TFM behavior; the fix applies identically on all TFMs
Verdict: Clean refactor, no issues.
🧠 Reviewed by expert-reviewer · PR #16074
🧠 Reviewed by Expert Code Reviewer 🧠
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.
Caution
This PR requires manual review because threat detection produced a warning.
Reason: parse_error
Review the workflow run logs for details.
This PR contains changes that were originally intended for PR #15788 (
fix/issue-15673-0ae77a4bb78cb877).Please review the changes carefully before merging.