Add new diagnostic env var names matching renamed CLI options#8388
Conversation
The CLI options `--diagnostic-output-fileprefix` and `--diagnostic-filelogger-synchronouswrite` were renamed to `--diagnostic-file-prefix` and `--diagnostic-synchronous-write`, but the corresponding env vars kept their old names. This adds new env var names that align with the new CLI option names, while keeping the legacy names working for backward compatibility. The new names take precedence; legacy names are documented as deprecated and can be removed in the next breaking-change version. Fixes #7159 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds new environment variable names for diagnostic settings to match the renamed CLI options, while preserving the legacy env var names for backward compatibility.
Changes:
- Introduces
TESTINGPLATFORM_DIAGNOSTIC_FILE_PREFIXandTESTINGPLATFORM_DIAGNOSTIC_SYNCHRONOUS_WRITEconstants (legacy constants noted as deprecated). - Updates diagnostic env var reading logic to prefer the new names and fall back to legacy names.
- Adds acceptance tests verifying the new env var names and precedence behavior.
Show a summary per file
| File | Description |
|---|---|
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/DiagnosticTests.cs | Adds new acceptance tests for the new env var names and precedence behavior. |
| src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs | Adds new env var constants and annotates legacy constants with deprecation comments. |
| src/Platform/Microsoft.Testing.Platform/Builder/TestApplication.cs | Prefers new env vars for diagnostic prefix / synchronous write with legacy fallback. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
When the legacy `TESTINGPLATFORM_DIAGNOSTIC_OUTPUT_FILEPREFIX` or `TESTINGPLATFORM_DIAGNOSTIC_FILELOGGER_SYNCHRONOUSWRITE` environment variables are detected, write a localized warning to the console pointing users at the new `TESTINGPLATFORM_DIAGNOSTIC_FILE_PREFIX` / `TESTINGPLATFORM_DIAGNOSTIC_SYNCHRONOUS_WRITE` names. The legacy names still work; the warning fires whenever they are set (including when overridden by the new name) so callers always know to migrate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Added a follow-up commit that emits a deprecation warning when the legacy diagnostic env vars are set.
|
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Expert Review — PR #8388
| # | Dimension | Verdict |
|---|---|---|
| 13 | Test Completeness & Coverage | 🟢 1 NIT |
✅ 20/21 dimensions clean.
- Test coverage: missing
SynchronousWrite_NewNameTakesPrecedencetest (symmetric to the existingCustomPrefix_NewNameTakesPrecedence)
Summary
The change is correct and well-structured. The fallback logic in TestApplication.cs correctly prefers the new env var names and falls back to the legacy names when the new ones are absent or empty. The EnvironmentVariableConstants.cs constants are internal, so no PublicAPI.Unshipped.txt update is needed and no [Obsolete] is required by policy (though it would be a helpful internal signal — optional).
The three new acceptance tests cover the happy paths for both new names and the precedence case for the file-prefix pair. The only gap is the missing symmetric precedence test for the synchronous-write pair (filed as a NIT inline comment above).
No blocking or major issues found.
Generated by Expert Code Review (on open) for issue #8388 · ● 5.1M
…c precedence test - Mark legacy diagnostic env var constants TESTINGPLATFORM_DIAGNOSTIC_OUTPUT_FILEPREFIX and TESTINGPLATFORM_DIAGNOSTIC_FILELOGGER_SYNCHRONOUSWRITE with [Obsolete(error: false)] so any new internal call site fails the build, and suppress the warnings only at the intentional back-compat fallback sites in TestApplication.cs and at the file-wide level in DiagnosticTests.cs which exercises the legacy names on purpose. - Add Diag_EnableWithEnvironmentVariables_SynchronousWrite_NewNameTakesPrecedence test, symmetric to the existing CustomPrefix_NewNameTakesPrecedence test, to lock in the precedence rule for the synchronous-write env var pair as well. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #7159.
The CLI options were renamed in #6165:
but the matching environment variables kept their old names, so the option and env var names are no longer consistent:
Renaming the env vars outright would be a breaking change, so this PR instead adds new env var names that align with the new CLI option names, while keeping the legacy names working.
Changes
EnvironmentVariableConstants.cs: addedTESTINGPLATFORM_DIAGNOSTIC_FILE_PREFIXandTESTINGPLATFORM_DIAGNOSTIC_SYNCHRONOUS_WRITE. Documented the legacy constants as deprecated (kept for back-compat, to be removed in the next breaking-change version).TestApplication.cs: when reading the diagnostic file prefix / synchronous write env vars, the new name is read first and the legacy name is used as a fallback. This means existing users on the legacy names are not affected, and the new names take precedence when both are set.DiagnosticTests.cs: kept the existing acceptance tests covering the legacy names so we don't lose back-compat coverage, and added three new tests:Diag_EnableWithEnvironmentVariables_CustomPrefix_NewName_SucceededDiag_EnableWithEnvironmentVariables_SynchronousWrite_NewName_SucceededDiag_EnableWithEnvironmentVariables_CustomPrefix_NewNameTakesPrecedenceNo public API surface area was added —
EnvironmentVariableConstantsisinternal(and is source-linked into the acceptance test project, so the new constants are automatically visible to tests).The documentation update referenced in the issue lives in
dotnet/docsand is tracked separately per the issue comment.