Skip to content

[demo] Repro #54613: --environment without launch profile - #9

Open
mthalman wants to merge 1 commit into
demo/54613-basefrom
demo/54613-head
Open

mthalman wants to merge 1 commit into
demo/54613-basefrom
demo/54613-head

Conversation

@mthalman

Copy link
Copy Markdown
Owner

Repro of dotnet#54613 (env vars not applied to test process without launch profile). Base branch carries the code-review skill. Demo for skill-vs-default CCR comparison.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts dotnet test (Microsoft Testing Platform path) process startup so that environment variables provided via --environment are applied even when no launch profile / launchSettings is present, matching the expected CLI contract and addressing the repro for dotnet#54613.

Changes:

  • Move application of TestOptions.EnvironmentVariables so it runs regardless of whether Module.LaunchSettings is present.
  • Preserve the precedence rule where command-line --environment values override launch profile environment variables (when present).

Comment on lines +115 to +119
// Env variables specified on command line override those specified in launch profile:
foreach (var (name, value) in TestOptions.EnvironmentVariables)
{
processStartInfo.Environment[name] = value;
}
@mthalman

Copy link
Copy Markdown
Owner Author

Skill vs. default Copilot review — --environment without launch profile

This PR faithfully reproduces the exact diff that Copilot reviewed on dotnet#54613 (commit 7a3b72e), but on a base branch that carries the code-review skill. The diff is identical; the only variable is whether the skill is loaded.

The change: moves the --environment override loop out of the if (launch profile) block so the values are applied to the spawned test process even when there is no launch profile. A real, user-observable behavioral fix to dotnet test (Microsoft Testing Platform), shipped to a servicing branch, with no test added.

Default Copilot review

On the original PR, default Copilot Code Review produced 0 inline findings.

Skill-backed review (this PR)

The skill produced one finding — a scenario-accurate regression-coverage gap:

This change fixes environment variable propagation when there is no launch profile, but there's no regression coverage ensuring --environment values are applied to the spawned MTP test process when Module.LaunchSettings is null (e.g., dotnet test --no-launch-profile -e KEY=VALUE, or when launchSettings.json is absent). Existing tests appear to cover env vars only when launch settings are present, so this could regress again unnoticed.

Why the difference

This is the skill's Impact Analysis for Tests and Regressions working as designed: rather than stopping at "the code change looks correct," it maps the changed code path to the behavior that could regress, compares that against existing coverage, and names the precise missing scenario — including the exact no-launch-profile invocation that would have failed before the fix. Default CCR reviewed the same diff and flagged nothing.

This is a different category of finding than the explicit-interface completeness gaps in the earlier comparison: there the skill went deeper on in-diff correctness; here it surfaces a missing regression test that a behavioral bug fix should carry. Both are cases where the default review was silent.

This branch has not been deployed

No deployments
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.

2 participants