MSTEST0005: Stop reporting on fields of type TestContext - #8629

Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged

MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom
dev/amauryleve/mstest0005-no-fields

Conversation

@Evangelink

@EvangelinkAmaury Levé (Evangelink) commented May 27, 2026

Copy link
Copy Markdown
Member

Summary

Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.

Fixes#4590.

Why

Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:

[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}

The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".

Trade-off

A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.

Changes

  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:
    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.

Validation

MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).

CopilotAI review requested due to automatic review settings May 27, 2026 10:31
CopilotAI reviewed May 27, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.

Changes:

  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.

Copilot's findings

  • Files reviewed: 20/20 changed files
  • Comments generated: 3

Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink) commented May 27, 2026
View reviewed changes

@EvangelinkAmaury Levé (Evangelink) left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries

IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.

Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.

Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • builds.dotnet.microsoft.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"

See Network Configuration for more information.

Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M

Comments that could not be inline-anchored

src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199

Defensive Coding — Missing exception handling on user callback

The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.

Recommendation:

// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>

@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed the dev/amauryleve/mstest0005-no-fields branch from 5844f3a to 050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink) enabled auto-merge (squash) May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink) merged commit 561ebf7 into mainMay 27, 2026
46 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/mstest0005-no-fields branch May 27, 2026 12:16
CopilotAI mentioned this pull request May 27, 2026
Merged
@EvangelinkAmaury Levé (Evangelink) mentioned this pull request Jun 10, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005

3 participants

@Evangelink@JanKrivanek
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
MSTEST0005: Stop reporting on fields of type TestContext - #8629
Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged
MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
Conversation
@Evangelink
@EvangelinkAmaury Levé (Evangelink)
commented
May 27, 2026

Copy link


Copy Markdown

Member
Summary
Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.
Fixes#4590.
Why
Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:
[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}
The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".
Trade-off
A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.
Changes
  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:

    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.
Validation
MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).
CopilotAI
review requested due to automatic review settings
May 27, 2026 10:31
CopilotAI
reviewed
May 27, 2026
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.
Changes:
  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.
Copilot's findings
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink)
commented
May 27, 2026
View reviewed changes
@EvangelinkAmaury Levé (Evangelink)
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries
IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.
Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.
Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
  • builds.dotnet.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"
See Network Configuration for more information.
Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M
Comments that could not be inline-anchored
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199
Defensive Coding — Missing exception handling on user callback
The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.
Recommendation:
// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>
@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed
the
dev/amauryleve/mstest0005-no-fields
branch
from
5844f3a to
050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink)
enabled auto-merge (squash)
May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink)
merged commit 561ebf7
into
mainMay 27, 2026

46 checks passed

@Evangelink
Amaury Levé (Evangelink)
deleted the

dev/amauryleve/mstest0005-no-fields

branch
May 27, 2026 12:16
CopilotAI
mentioned this pull request
May 27, 2026
 Merged

@EvangelinkAmaury Levé (Evangelink)
mentioned this pull request
Jun 10, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005


3 participants

@Evangelink@JanKrivanek

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
MSTEST0005: Stop reporting on fields of type TestContext - #8629
Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged
MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
Conversation
@Evangelink
@EvangelinkAmaury Levé (Evangelink)
commented
May 27, 2026

Copy link


Copy Markdown

Member
Summary
Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.
Fixes#4590.
Why
Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:
[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}
The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".
Trade-off
A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.
Changes
  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:

    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.
Validation
MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).
CopilotAI
review requested due to automatic review settings
May 27, 2026 10:31
CopilotAI
reviewed
May 27, 2026
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.
Changes:
  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.
Copilot's findings
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink)
commented
May 27, 2026
View reviewed changes
@EvangelinkAmaury Levé (Evangelink)
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries
IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.
Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.
Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
  • builds.dotnet.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"
See Network Configuration for more information.
Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M
Comments that could not be inline-anchored
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199
Defensive Coding — Missing exception handling on user callback
The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.
Recommendation:
// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>
@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed
the
dev/amauryleve/mstest0005-no-fields
branch
from
5844f3a to
050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink)
enabled auto-merge (squash)
May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink)
merged commit 561ebf7
into
mainMay 27, 2026

46 checks passed

@Evangelink
Amaury Levé (Evangelink)
deleted the

dev/amauryleve/mstest0005-no-fields

branch
May 27, 2026 12:16
CopilotAI
mentioned this pull request
May 27, 2026
 Merged

@EvangelinkAmaury Levé (Evangelink)
mentioned this pull request
Jun 10, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005


3 participants

@Evangelink@JanKrivanek

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
MSTEST0005: Stop reporting on fields of type TestContext - #8629
Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged
MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
Conversation
@Evangelink
@EvangelinkAmaury Levé (Evangelink)
commented
May 27, 2026

Copy link


Copy Markdown

Member
Summary
Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.
Fixes#4590.
Why
Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:
[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}
The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".
Trade-off
A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.
Changes
  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:

    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.
Validation
MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).
CopilotAI
review requested due to automatic review settings
May 27, 2026 10:31
CopilotAI
reviewed
May 27, 2026
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.
Changes:
  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.
Copilot's findings
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink)
commented
May 27, 2026
View reviewed changes
@EvangelinkAmaury Levé (Evangelink)
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries
IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.
Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.
Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
  • builds.dotnet.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"
See Network Configuration for more information.
Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M
Comments that could not be inline-anchored
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199
Defensive Coding — Missing exception handling on user callback
The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.
Recommendation:
// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>
@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed
the
dev/amauryleve/mstest0005-no-fields
branch
from
5844f3a to
050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink)
enabled auto-merge (squash)
May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink)
merged commit 561ebf7
into
mainMay 27, 2026

46 checks passed

@Evangelink
Amaury Levé (Evangelink)
deleted the

dev/amauryleve/mstest0005-no-fields

branch
May 27, 2026 12:16
CopilotAI
mentioned this pull request
May 27, 2026
 Merged

@EvangelinkAmaury Levé (Evangelink)
mentioned this pull request
Jun 10, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005


3 participants

@Evangelink@JanKrivanek

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
MSTEST0005: Stop reporting on fields of type TestContext - #8629
Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged
MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
Conversation
@Evangelink
@EvangelinkAmaury Levé (Evangelink)
commented
May 27, 2026

Copy link


Copy Markdown

Member
Summary
Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.
Fixes#4590.
Why
Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:
[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}
The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".
Trade-off
A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.
Changes
  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:

    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.
Validation
MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).
CopilotAI
review requested due to automatic review settings
May 27, 2026 10:31
CopilotAI
reviewed
May 27, 2026
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.
Changes:
  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.
Copilot's findings
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink)
commented
May 27, 2026
View reviewed changes
@EvangelinkAmaury Levé (Evangelink)
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries
IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.
Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.
Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
  • builds.dotnet.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"
See Network Configuration for more information.
Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M
Comments that could not be inline-anchored
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199
Defensive Coding — Missing exception handling on user callback
The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.
Recommendation:
// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>
@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed
the
dev/amauryleve/mstest0005-no-fields
branch
from
5844f3a to
050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink)
enabled auto-merge (squash)
May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink)
merged commit 561ebf7
into
mainMay 27, 2026

46 checks passed

@Evangelink
Amaury Levé (Evangelink)
deleted the

dev/amauryleve/mstest0005-no-fields

branch
May 27, 2026 12:16
CopilotAI
mentioned this pull request
May 27, 2026
 Merged

@EvangelinkAmaury Levé (Evangelink)
mentioned this pull request
Jun 10, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005


3 participants

@Evangelink@JanKrivanek

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
MSTEST0005: Stop reporting on fields of type TestContext - #8629
Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged
MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
Conversation
@Evangelink
@EvangelinkAmaury Levé (Evangelink)
commented
May 27, 2026

Copy link


Copy Markdown

Member
Summary
Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.
Fixes#4590.
Why
Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:
[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}
The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".
Trade-off
A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.
Changes
  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:

    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.
Validation
MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).
CopilotAI
review requested due to automatic review settings
May 27, 2026 10:31
CopilotAI
reviewed
May 27, 2026
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.
Changes:
  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.
Copilot's findings
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink)
commented
May 27, 2026
View reviewed changes
@EvangelinkAmaury Levé (Evangelink)
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries
IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.
Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.
Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
  • builds.dotnet.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"
See Network Configuration for more information.
Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M
Comments that could not be inline-anchored
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199
Defensive Coding — Missing exception handling on user callback
The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.
Recommendation:
// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>
@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed
the
dev/amauryleve/mstest0005-no-fields
branch
from
5844f3a to
050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink)
enabled auto-merge (squash)
May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink)
merged commit 561ebf7
into
mainMay 27, 2026

46 checks passed

@Evangelink
Amaury Levé (Evangelink)
deleted the

dev/amauryleve/mstest0005-no-fields

branch
May 27, 2026 12:16
CopilotAI
mentioned this pull request
May 27, 2026
 Merged

@EvangelinkAmaury Levé (Evangelink)
mentioned this pull request
Jun 10, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005


3 participants

@Evangelink@JanKrivanek

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
MSTEST0005: Stop reporting on fields of type TestContext - #8629
Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged
MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
Conversation
@Evangelink
@EvangelinkAmaury Levé (Evangelink)
commented
May 27, 2026

Copy link


Copy Markdown

Member
Summary
Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.
Fixes#4590.
Why
Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:
[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}
The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".
Trade-off
A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.
Changes
  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:

    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.
Validation
MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).
CopilotAI
review requested due to automatic review settings
May 27, 2026 10:31
CopilotAI
reviewed
May 27, 2026
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.
Changes:
  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.
Copilot's findings
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink)
commented
May 27, 2026
View reviewed changes
@EvangelinkAmaury Levé (Evangelink)
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries
IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.
Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.
Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
  • builds.dotnet.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"
See Network Configuration for more information.
Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M
Comments that could not be inline-anchored
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199
Defensive Coding — Missing exception handling on user callback
The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.
Recommendation:
// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>
@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed
the
dev/amauryleve/mstest0005-no-fields
branch
from
5844f3a to
050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink)
enabled auto-merge (squash)
May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink)
merged commit 561ebf7
into
mainMay 27, 2026

46 checks passed

@Evangelink
Amaury Levé (Evangelink)
deleted the

dev/amauryleve/mstest0005-no-fields

branch
May 27, 2026 12:16
CopilotAI
mentioned this pull request
May 27, 2026
 Merged

@EvangelinkAmaury Levé (Evangelink)
mentioned this pull request
Jun 10, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005


3 participants

@Evangelink@JanKrivanek

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
MSTEST0005: Stop reporting on fields of type TestContext - #8629
Merged
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
May 27, 2026
Merged
MSTEST0005: Stop reporting on fields of type TestContext#8629
Amaury Levé (Evangelink) merged 1 commit into
mainfrom 
dev/amauryleve/mstest0005-no-fields
Conversation
@Evangelink
@EvangelinkAmaury Levé (Evangelink)
commented
May 27, 2026

Copy link


Copy Markdown

Member
Summary
Removes the field-handling branch from TestContextShouldBeValidAnalyzer so MSTEST0005 only validates the TestContext property layout. The rule's message reads "Property 'TestContext' should be valid", so the field path was already at odds with the diagnostic text.
Fixes#4590.
Why
Reporting on every static or unassigned field of type TestContext produced too many false positives. The most visible one comes from a perfectly reasonable pattern:
[TestClass]publicclassMyTests{privatestaticTestContext_context;[ClassInitialize]publicstaticvoidClassInitialize(TestContextcontext)=>_context=context;}
The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with the much clearer message "Do not store TestContext in a static member".
Trade-off
A static field of type TestContext that is never assigned now gets no MSTEST0005 (and no MSTEST0024 since there is no assignment site). This is an accepted trade-off in favor of removing the false positives; a regression test (WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic) documents the behavior so it isn't accidentally restored.
Changes
  • src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.cs — replace the switch (member.Kind) over property/field with an IPropertySymbol filter; property analysis (auto-assignment, constructor assignment, backing-field fallback) is unchanged.
  • src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.cs — drop ConvertFieldToProperty and the FieldDeclarationSyntax branch; the fixer now operates on property declarations only.
  • src/Analyzers/MSTest.Analyzers/Resources.resx — update TestContextShouldBeValidDescription to drop the mention of "field". XLF files regenerated via UpdateXlf.
  • test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.cs:

    • WhenTestContextCaseInsensitiveIsField_Diagnostic → WhenTestContextCaseInsensitiveIsField_NoDiagnostic (asserts no diagnostic, same parametric cases).
    • Drops the now-redundant WhenTestContextCaseInsensitiveIsField_AssignedInConstructor_NoDiagnostic.
    • Adds WhenStaticFieldOfTypeTestContextAssignedInClassInitialize_NoDiagnostic covering the exact Private static field causes false positive for analyzer rule MSTEST0005 #4590 scenario.
    • Adds WhenStaticFieldOfTypeTestContextIsNeverAssigned_NoDiagnostic documenting the deliberate trade-off.
Validation
MSTest.Analyzers.UnitTests passes on both net8.0 and net472 (66 tests for TestContextShouldBeValidAnalyzerTests).
CopilotAI
review requested due to automatic review settings
May 27, 2026 10:31
CopilotAI
reviewed
May 27, 2026
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
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 updates the MSTest analyzers so MSTEST0005 no longer reports diagnostics for fields of type TestContext, and instead only validates the TestContext property layout (aligning behavior with the rule’s diagnostic wording and reducing false positives like #4590). It also includes unrelated IPC and reviewer-guidance changes.
Changes:
  • Update TestContextShouldBeValidAnalyzer to analyze only TestContextproperties (skip fields entirely).
  • Simplify TestContextShouldBeValidFixer to operate on property declarations only and remove field-to-property conversion.
  • Update resources + localization and adjust/add unit tests covering the regression and the accepted trade-off.
Show a summary per file
FileDescription
test/UnitTests/MSTest.Analyzers.UnitTests/TestContextShouldBeValidAnalyzerTests.csUpdates/extends tests to ensure fields (including static ones) are no longer flagged by MSTEST0005; adds regression coverage for #4590.
src/Analyzers/MSTest.Analyzers/TestContextShouldBeValidAnalyzer.csRemoves field handling and filters analysis to IPropertySymbol named TestContext (case-insensitive).
src/Analyzers/MSTest.Analyzers.CodeFixes/TestContextShouldBeValidFixer.csRemoves field conversion path; fixer now normalizes and completes TestContext properties only.
src/Analyzers/MSTest.Analyzers/Resources.resxUpdates rule description text to remove mention of fields.
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlfRegenerates localization entry for updated description (marks translation as needs review).
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.csAdds disconnect/debug logging and treats write-time pipe breaks as graceful disconnects; adds timeout logging in dispose paths.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.csAdds handling for server disconnect during write (exit + rethrow).
.github/agents/expert-reviewer.agent.mdAdds guidance discouraging weakening invariant guards without evidence.
Copilot's findings
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs
Comment thread.github/agents/expert-reviewer.agent.md
Comment threadsrc/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs
Evangelink
Amaury Levé (Evangelink)
commented
May 27, 2026
View reviewed changes
@EvangelinkAmaury Levé (Evangelink)
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Dimension 8: Defensive Coding at Boundaries
IPC Exception Handling: ✅ The new IOException/ObjectDisposedException catches in NamedPipeClient.cs and NamedPipeServer.cs are correctly placed at trust boundaries. They handle concrete external triggers (peer process crash, OS-level pipe closure), not internal invariants. This defensive coding is appropriate.
Issue Found: ❌ Line 199 in NamedPipeServer.cs invokes the user-provided _callback without exception handling, violating Rule #1. See inline comment for details.
Analyzer Changes: The MSTEST0005 changes (removing field handling from TestContextShouldBeValidAnalyzer) do not involve trust boundaries or defensive coding concerns.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
  • builds.dotnet.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "builds.dotnet.microsoft.com"
See Network Configuration for more information.
Generated by Expert Code Review (on open) for issue #8629 · ● 34.5M
Comments that could not be inline-anchored
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:199
Defensive Coding — Missing exception handling on user callback
The user-provided _callback is invoked without exception handling. If the callback throws (e.g., due to a bug in the handler or unexpected request type), the server loop will crash and potentially take down the host process.
Recommendation:
// Call the callbackIResponseresponse;try{response=await_callback(deserializedObject).ConfigureAwait(false);}catch(Exceptionex){// Log the exception a…</details>
@Evangelink
MSTEST0005: Stop reporting on fields of type TestContext
050afca
Remove the field-handling branch from TestContextShouldBeValidAnalyzer. The rule is now property-only, matching its diagnostic message (Property 'TestContext' should be valid).
Reporting on every static or unassigned field of type TestContext produced too many false positives (see #4590). The static-field misuse case is already covered by MSTEST0024 (DoNotStoreStaticTestContextAnalyzer), which warns at the assignment site with a clearer message.
Also simplify the code fixer (drop the field-to-property conversion path) and the rule description, and add regression tests for the #4590 scenario.
Fixes#4590.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink)force-pushed
the
dev/amauryleve/mstest0005-no-fields
branch
from
5844f3a to
050afcaCompareMay 27, 2026 11:13
@Evangelink
Amaury Levé (Evangelink)
enabled auto-merge (squash)
May 27, 2026 12:06
@Evangelink
Amaury Levé (Evangelink)
merged commit 561ebf7
into
mainMay 27, 2026

46 checks passed

@Evangelink
Amaury Levé (Evangelink)
deleted the

dev/amauryleve/mstest0005-no-fields

branch
May 27, 2026 12:16
CopilotAI
mentioned this pull request
May 27, 2026
 Merged

@EvangelinkAmaury Levé (Evangelink)
mentioned this pull request
Jun 10, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@JanKrivanekJan Krivanek (JanKrivanek)JanKrivanek approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Private static field causes false positive for analyzer rule MSTEST0005


3 participants

@Evangelink@JanKrivanek