Skip to content

Sync with TemplateDotNetLibrary template updates#114

Merged
Malcolmnixon merged 4 commits intomainfrom
copilot/pull-template-repo-updates
Mar 6, 2026
Merged

Sync with TemplateDotNetLibrary template updates#114
Malcolmnixon merged 4 commits intomainfrom
copilot/pull-template-repo-updates

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 6, 2026

Pull Request

Description

Pulls in updates from the TemplateDotNetLibrary template to reduce drift.

Workflow (build.yaml)

  • Added macos-latest to the build matrix
  • Simplified OS_SHORT sed pattern to s/-latest// (handles any OS automatically)
  • Added build-mode: manual to CodeQL init step

Source project (DemaConsulting.TestResults.csproj)

  • Added Polyfill v9.13.0 (PrivateAssets="All") and <PolyArgumentExceptions>true</PolyArgumentExceptions>
  • Added <InternalsVisibleTo Include="DemaConsulting.TestResults.Tests" />

Test project

  • Added <Using Include="Polyfills" /> implicit using (forward-looking; also ensures tests on net481 can use polyfill helpers directly if needed)
  • Added [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("DynamicProxyGenAssembly2")] to AssemblyInfo.cs using a fully-qualified attribute name to avoid an ExecutionScope ambiguity with Microsoft.VisualStudio.TestTools.UnitTesting on net481

Serializer improvements (enabled by Polyfill)

  • Replaced _ = results ?? throw new ArgumentNullException(nameof(results)) with ArgumentNullException.ThrowIfNull(results) in JUnitSerializer.Serialize and TrxSerializer.Serialize
  • Replaced multi-line if (string.IsNullOrWhiteSpace(...)) { throw ... } with ArgumentException.ThrowIfNullOrWhiteSpace(...) in JUnitSerializer.Deserialize, TrxSerializer.Deserialize, and Serializer.Deserialize
  • Updated XML doc <exception> comments to separately document ArgumentNullException (null) and ArgumentException (whitespace) cases
  • Updated null-input Deserialize tests from ThrowsExactly<ArgumentException> to ThrowsExactly<ArgumentNullException> to match the corrected exception type (ThrowIfNullOrWhiteSpace throws ArgumentNullException for null)

Tooling / docs

  • Added missing .github/ISSUE_TEMPLATE/question.yml
  • Added 5th MSTest best practice (Assert.StartsWith) to test-developer.md
  • Rewrote AGENTS.md to the new concise quick-reference format

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code quality improvement

Related Issues

Pre-Submission Checklist

Before submitting this pull request, ensure you have completed the following:

Build and Test

  • Code builds successfully: dotnet build --configuration Release
  • All unit tests pass: dotnet test --configuration Release
  • Code produces zero warnings

Code Quality

  • Code formatting is correct: dotnet format --verify-no-changes
  • New code has appropriate XML documentation comments
  • Static analyzer warnings have been addressed

Quality Checks

Please run the following checks before submitting:

  • Spell checker passes: cspell "**/*.{md,cs}"
  • Markdown linter passes: markdownlint "**/*.md"
  • YAML linter passes: yamllint .

Testing

  • Added unit tests for new functionality
  • Updated existing tests if behavior changed
  • All tests follow the AAA (Arrange, Act, Assert) pattern
  • Test coverage is maintained or improved

Documentation

  • Updated README.md (if applicable)
  • Updated ARCHITECTURE.md (if applicable)
  • Added code examples for new features (if applicable)
  • Updated requirements.yaml (if applicable)

Additional Notes

The Polyfill package is PrivateAssets="All" — it does not become a transitive dependency of library consumers.

On net481, ExecutionScope exists in both System.Runtime.CompilerServices and Microsoft.VisualStudio.TestTools.UnitTesting. Using a fully-qualified System.Runtime.CompilerServices.InternalsVisibleTo attribute (rather than a using import) avoids the ambiguity without affecting other target frameworks.

ArgumentException.ThrowIfNullOrWhiteSpace throws ArgumentNullException for null inputs and ArgumentException for empty/whitespace inputs. Null-input Deserialize tests were updated accordingly to expect ArgumentNullException rather than ArgumentException.

The <Using Include="Polyfills" /> in the test project does not cause build errors or warnings (verified with TreatWarningsAsErrors=true and EnforceCodeStyleInBuild=true). On .NET 8/9/10 the Polyfills namespace has no additional types (those APIs are native to the BCL), so the import is silently a no-op while remaining available for future use on net481.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copilot AI changed the title [WIP] Update branch with latest template repository changes Sync with TemplateDotNetLibrary template updates Mar 6, 2026
@Malcolmnixon
Copy link
Copy Markdown
Contributor

@copilot Got the following problem:

Error: D:\a\TestResults\TestResults\test\DemaConsulting.TestResults.Tests\AssemblyInfo.cs(25,45): error CS0104: 'ExecutionScope' is an ambiguous reference between 'System.Runtime.CompilerServices.ExecutionScope' and 'Microsoft.VisualStudio.TestTools.UnitTesting.ExecutionScope' [D:\a\TestResults\TestResults\test\DemaConsulting.TestResults.Tests\DemaConsulting.TestResults.Tests.csproj::TargetFramework=net481]

Please analyze the change you did and see if it's appropriate.

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot Got the following problem:

Error: D:\a\TestResults\TestResults\test\DemaConsulting.TestResults.Tests\AssemblyInfo.cs(25,45): error CS0104: 'ExecutionScope' is an ambig...

The issue was that importing using System.Runtime.CompilerServices; caused ExecutionScope to become ambiguous on net481 (it exists in both that namespace and Microsoft.VisualStudio.TestTools.UnitTesting).

Fixed in a6171c7 by dropping the using import and using the fully-qualified System.Runtime.CompilerServices.InternalsVisibleTo attribute name instead, keeping ExecutionScope unambiguously resolved from the MSTest namespace.

@Malcolmnixon Malcolmnixon marked this pull request as ready for review March 6, 2026 18:45
Copilot AI review requested due to automatic review settings March 6, 2026 18:45
@Malcolmnixon
Copy link
Copy Markdown
Contributor

@copilot Now that the Polyfills library is present, are there any language features that should be used that were perhaps avoided due to older language versions or missing runtime functionality?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Syncs this repository with upstream TemplateDotNetLibrary updates to reduce drift, covering CI workflow enhancements, project configuration changes, and tooling/documentation additions.

Changes:

  • Expands CI build matrix to include macOS, simplifies OS job ID generation, and configures CodeQL to use manual build mode.
  • Updates project files to add Polyfill configuration and expose internals to the test assembly.
  • Refreshes agent/docs assets and adds a GitHub “Question/Help” issue form.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/DemaConsulting.TestResults.Tests/DemaConsulting.TestResults.Tests.csproj Adds an implicit global using for Polyfills in the test project.
test/DemaConsulting.TestResults.Tests/AssemblyInfo.cs Adds InternalsVisibleTo for DynamicProxy to support proxy-based mocking scenarios.
src/DemaConsulting.TestResults/DemaConsulting.TestResults.csproj Adds Polyfill configuration and InternalsVisibleTo for the test project.
AGENTS.md Rewrites agent guidance into a concise quick-reference format.
.github/workflows/build.yaml Adds macOS to the build matrix, simplifies OS short-name logic, and sets CodeQL build-mode: manual.
.github/agents/test-developer.md Adds an MSTest best-practice note encouraging Assert.StartsWith.
.github/ISSUE_TEMPLATE/question.yml Adds a “Question / Help” GitHub issue form template.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot Now that the Polyfills library is present, are there any language features that should be used that were perhaps avoided due to older language versions or missing runtime functionality?

Yes — two argument validation patterns could be improved now that PolyArgumentExceptions=true is active (commit 942a714):

  1. _ = results ?? throw new ArgumentNullException(nameof(results))ArgumentNullException.ThrowIfNull(results) (used in JUnitSerializer.Serialize and TrxSerializer.Serialize)

  2. Multi-line if (string.IsNullOrWhiteSpace(...)) { throw new ArgumentException(...) }ArgumentException.ThrowIfNullOrWhiteSpace(...) (used in JUnitSerializer.Deserialize, TrxSerializer.Deserialize, and Serializer.Deserialize)

One subtlety: ThrowIfNullOrWhiteSpace throws ArgumentNullException (not ArgumentException) for null inputs, so the three Deserialize null-input tests were updated from ThrowsExactly<ArgumentException> to ThrowsExactly<ArgumentNullException>, and the XML doc exception comments were split into separate ArgumentNullException/ArgumentException entries.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants