[Test Improver] Add unit tests for ResponseFileHelper.TryReadResponseFile#8374
Conversation
Cover six scenarios: file not found (returns false, adds error with path), valid file with multiple lines (returns true, tokenizes args), lines starting with '#' are treated as comments (skipped), blank/ whitespace lines are skipped, quoted arguments are unquoted correctly, and an empty file returns true with an empty array. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds missing unit test coverage for ResponseFileHelper.TryReadResponseFile, which is used to expand @response-file command-line arguments in Microsoft.Testing.Platform.
Changes:
- Added unit tests covering response-file read behavior for missing files, empty files, multi-line content, comment lines, blank/whitespace lines, and quoted arguments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/ResponseFileHelperTests.cs | Adds new TryReadResponseFile unit tests to validate response-file parsing and error reporting behavior. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
Evangelink
left a comment
There was a problem hiding this comment.
Good addition of six well-structured TryReadResponseFile tests. All use proper temp-file lifecycle (try/finally + File.Delete), unique paths, and consistent MSTest assertion APIs.
| # | Dimension | Verdict |
|---|---|---|
| 11 | Assertion Quality | 🟡 2 NIT |
| 13 | Test Completeness & Coverage | 🟡 1 MODERATE |
✅ 19/21 dimensions clean.
Assertion Quality (NIT): Two success-path tests (FileWithComments, FileWithBlankLines) are missing Assert.IsEmpty(errors). The other two success-path tests both include this assertion — the gap is inconsistent and would let a regression that emits spurious errors slip through.
Test Completeness (MODERATE): The IOException branch in TryReadResponseFile (lines 22–24 of ResponseFileHelper.cs) has no test coverage. It produces a different error message (CommandLineParserFailedToReadResponseFile) than the FileNotFoundException branch. Consider adding a test that mocks or creates an unreadable file to exercise this path, or at minimum add a tracking comment/issue.
Generated by Expert Code Review (on open) for issue #8374 · ● 4.8M
…xception coverage - Use GetTempFileName + Delete to deterministically guarantee non-existence in FileNotFound test (avoids potential collisions/leftover files). - Add Assert.IsEmpty(errors) to FileWithComments and FileWithBlankLines success-path tests for consistency with other passing-path assertions. - Add new Windows-only TryReadResponseFile_FileLocked_ReturnsFalseAndAddsError test exercising the IOException catch branch (CommandLineParserFailedToReadResponseFile). Uses [OSCondition(OperatingSystems.Windows)] since FileShare.None is only honored on Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the MODERATE finding about the uncovered |
Cover six scenarios: file not found (returns false, adds error with
path), valid file with multiple lines (returns true, tokenizes args),
lines starting with '#' are treated as comments (skipped), blank/
whitespace lines are skipped, quoted arguments are unquoted correctly,
and an empty file returns true with an empty array.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
Fixes #8372