Skip to content

Fixes from reviews.#145

Merged
Malcolmnixon merged 11 commits into
mainfrom
review-fixes
Apr 24, 2026
Merged

Fixes from reviews.#145
Malcolmnixon merged 11 commits into
mainfrom
review-fixes

Conversation

@Malcolmnixon

Copy link
Copy Markdown
Member

Pull Request

Description

This PR applies fixes from performing the formal reviews.

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

Closes #

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:

  • All linters pass: ./lint.sh (Unix/macOS) or cmd /c lint.bat / ./lint.bat (Windows)

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

Copilot AI review requested due to automatic review settings April 23, 2026 23:17

Copilot AI 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 applies review-driven cleanups across tests and documentation to improve standards alignment and traceability descriptions in the TestResults library repo.

Changes:

  • Update MSTest files to use explicit AAA section comments (Arrange:, Act:, Assert:).
  • Refine requirements/design documentation structure references (ReqStream and design intro).
  • Adjust README content (outcome categorization, script usage guidance, architecture link).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/DemaConsulting.TestResults.Tests/IO/TrxSerializerTests.cs Updates test comments to explicit AAA sections for consistency with testing standards.
test/DemaConsulting.TestResults.Tests/IO/SerializerTests.cs Updates test comments to explicit AAA sections for consistency with testing standards.
docs/reqstream/test-results-library/io/io.yaml Updates the comment header list of unit requirement files (now excludes serializer-helpers).
docs/design/introduction.md Adds a “Companion Artifact Structure” section and updates reference links.
README.md Reorders Completed outcome category, updates script commands to PowerShell, and repoints the architecture link to design introduction.

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

Comment thread docs/design/introduction.md
Comment thread docs/reqstream/test-results-library/io/io.yaml
Comment thread docs/design/introduction.md
Malcolm Nixon and others added 3 commits April 23, 2026 19:49
- requirements.yaml: add missing serializer-helpers.yaml include so that
  TestResults-SerializerHelpers-Utf8Encoding is part of the requirements hierarchy
- docs/design/introduction.md: update companion artifact structure to include
  serializer-helpers.yaml, SerializerHelpersTests.cs, and IOTests.cs
- docs/design/test-results-library/io/serializer.md: correct SerializerHelpers
  Dependency section — it is TrxSerializer.Serialize() and JUnitSerializer.Serialize()
  that use SerializerHelpers, not Serializer.Deserialize()
- src/DemaConsulting.TestResults/IO/SerializerHelpers.cs: improve XmlDoc to
  explain WHY the class exists and WHY the Encoding override is needed,
  per literate coding standards

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 24, 2026 00:10

Copilot AI 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

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


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

Comment thread docs/design/test-results-library/io/serializer-helpers.md Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 24, 2026 00:14
Links to reqstream YAML files from design markdown documents break when
compiled to PDF. The relationship between design and requirements is
captured structurally in the review-sets and requirements hierarchy.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI 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

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.


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

Comment thread test/DemaConsulting.TestResults.Tests/IO/SerializerTests.cs Outdated
Comment thread test/DemaConsulting.TestResults.Tests/IO/TrxSerializerTests.cs
Comment thread test/DemaConsulting.TestResults.Tests/IO/TrxSerializerTests.cs
Comment thread test/DemaConsulting.TestResults.Tests/IO/TrxSerializerTests.cs
Malcolm Nixon and others added 2 commits April 23, 2026 20:18
Copilot AI review requested due to automatic review settings April 24, 2026 00:19
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Malcolmnixon

Copy link
Copy Markdown
Member Author

@copilot apply changes based on the comments in this thread

…sert section

Agent-Logs-Url: https://github.com/demaconsulting/TestResults/sessions/a73f65e7-afde-4f3a-bed6-47904632ae6f

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>

Copilot AI commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

@copilot apply changes based on the comments in this thread

Applied the two remaining unresolved fixes from that review: in TrxSerializerTests.cs, moved Assert.IsNotNull(result) out of the // Act: section and into a new // Assert: verify the serialized result is present section for both affected test methods (the single-test and stack-trace serialize tests). Commit c36928d.

Copilot AI 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

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.


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

Comment thread src/DemaConsulting.TestResults/IO/SerializerHelpers.cs Outdated
Comment thread docs/reqstream/test-results-library/io/serializer-helpers.yaml Outdated
@Malcolmnixon

Copy link
Copy Markdown
Member Author

@copilot apply changes based on the comments in this thread

…nd docs

Agent-Logs-Url: https://github.com/demaconsulting/TestResults/sessions/bc3d5ab9-dbb5-4a05-af67-af33590e3357

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>

Copilot AI commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

@copilot apply changes based on the comments in this thread

Applied: .NET's Encoding.UTF8.WebName returns "utf-8" (lowercase), so the actual output is always lowercase. Updated SerializerHelpers.cs comments, serializer-helpers.yaml justification, and serializer-helpers.md design doc to use encoding="utf-8" consistently. Commit 0b5b8b7.

@Malcolmnixon Malcolmnixon merged commit ed898d2 into main Apr 24, 2026
8 checks passed
@Malcolmnixon Malcolmnixon deleted the review-fixes branch April 24, 2026 00:43
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