Skip to content

Remove "Subsystem" from subsystem test suite class and file names#64

Merged
Malcolmnixon merged 3 commits intomainfrom
copilot/remove-subsystem-from-tests
Apr 7, 2026
Merged

Remove "Subsystem" from subsystem test suite class and file names#64
Malcolmnixon merged 3 commits intomainfrom
copilot/remove-subsystem-from-tests

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 7, 2026

Pull Request

Description

Subsystem test files and classes had redundant "Subsystem" in their names (e.g. CliSubsystemTests). Removed the suffix per naming conventions.

Changes

  • Renamed files (git mv): *SubsystemTests.cs*Tests.cs across all 6 subsystem test files
    • CliSubsystemTests.csCliTests.cs
    • CaptureSubsystemTests.csCaptureTests.cs
    • ConfigurationSubsystemTests.csConfigurationTests.cs
    • ConfigurationLoadSubsystemTests.cs merged into ConfigurationTests.cs (see below)
    • PublishingSubsystemTests.csPublishingTests.cs
    • SelfTestSubsystemTests.csSelfTestTests.cs
  • Updated class names and test method name prefixes to match (e.g. CliSubsystem_Run_*Cli_Run_*)
  • Updated .reviewmark.yaml file path references to match new names
  • Merged and cleaned up ConfigurationLoadTests.cs: The original ConfigurationLoadSubsystemTests.cs content was merged into ConfigurationTests.cs, then the over-reaching Configuration_Run_* tests (which called Program.Run() — the Cli layer — rather than the Configuration subsystem's own API) were removed. ConfigurationTests.cs now contains only Configuration_ReadFromFile_* tests that exercise the Configuration subsystem directly via VersionMarkConfig.
  • Fixed all reqstream YAML files: All six subsystem reqstream files (cli/cli.yaml, capture/capture.yaml, configuration/configuration.yaml, configuration/load.yaml, publishing/publishing.yaml, self-test/self-test.yaml) were updated to reference the current test method names (the stale *Subsystem_* names had not been updated during the initial rename). The load.yaml tool-level requirements were also corrected to reference the proper VersionMarkConfig_Load_* unit tests in VersionMarkConfigLoadTests.cs instead of the now-removed Configuration_Run_* tests.

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
  • Self-validation tests pass:
    dotnet run --project src/DemaConsulting.VersionMark --configuration Release --framework net10.0
    --no-build -- --validate
  • 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

Pure rename with organizational cleanup — no logic changes. 186/186 tests pass across net8.0, net9.0, and net10.0.

The 9 removed Configuration_Run_* tests were redundant: the --lint CLI pipeline is already covered at the system level by Cli_Run_LintFlag_ValidConfig_Succeeds in CliTests.cs, and every individual load-validation behavior is already proven at the subsystem API level by the VersionMarkConfig_Load_* tests in VersionMarkConfigLoadTests.cs.

Rename test files and classes by removing 'Subsystem' from names:
- PublishingSubsystemTests.cs -> PublishingTests.cs
- CliSubsystemTests.cs -> CliTests.cs
- CaptureSubsystemTests.cs -> CaptureTests.cs
- SelfTestSubsystemTests.cs -> SelfTestTests.cs
- ConfigurationLoadSubsystemTests.cs -> ConfigurationLoadTests.cs
- ConfigurationSubsystemTests.cs -> ConfigurationTests.cs

Update class names and method name prefixes accordingly.
Update .reviewmark.yaml file path references to match new names.

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

@copilot What is the purpose of the Configuration xxxLoadTests.cs - why are the load tests different from the normal tests? This seems to be some convention only present in this folder.

Can you make sure the ConfigurationTests.cs (the subsystem tests for the Configuration subsystem) proves the Configuration subsystem meets its requirements by using the units in the Configuration subsystem.

Can you make sure the VersionMarkConfigTests.cs (the unit tests for the VersionMarkConfig unit) proves the unit meets its requirements by unit-testing the VersionMarkConfig class.

…into ConfigurationTests

Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/7af8f192-07fc-417c-87af-a6a4de4ace26

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

Copilot AI commented Apr 7, 2026

@copilot What is the purpose of the Configuration xxxLoadTests.cs - why are the load tests different from the normal tests? This seems to be some convention only present in this folder.

Can you mak...

ConfigurationLoadTests.cs (originally ConfigurationLoadSubsystemTests.cs) was a second subsystem test class that tested the --lint CLI pipeline end-to-end via Program.Run. It was separate from ConfigurationTests.cs because the Configuration folder happened to have two subsystem test classes — an unusual pattern not seen in any other subsystem.

I've addressed this in commit ee2e477:

  1. Merged ConfigurationLoadTests.cs into ConfigurationTests.cs — the --lint pipeline tests (Configuration_Run_*) are now alongside the configuration reading tests (Configuration_ReadFromFile_*), making ConfigurationTests.cs the single subsystem test file for the Configuration subsystem.

  2. ConfigurationTests.cs now proves the Configuration subsystem meets its requirements by exercising both its units: the ReadFromFile tests use VersionMarkConfig directly, and the Run tests exercise VersionMarkConfig.Load (which uses both VersionMarkConfig and LintIssue together) through the full --lint pipeline.

  3. VersionMarkConfigTests.cs already properly unit-tests the VersionMarkConfig class (covering ReadFromFile, FindVersions, ToolConfig methods etc.) and was not changed.

I also fixed a separate issue that was exposed: all six reqstream YAML files still referenced the old *Subsystem_* test method names from before the rename. Those have all been updated to match the current method names.

@Malcolmnixon
Copy link
Copy Markdown
Member

@copilot Subsystem tests are supposed to limit themselves to testing via subsystem functionality. Why is the Run() testing necessary? Is the testing at the system level not sufficient, or is there an over-reach in the requirements/justification?

…e load.yaml to subsystem-level tests

Agent-Logs-Url: https://github.com/demaconsulting/VersionMark/sessions/008c24a9-f309-4ed4-8a38-98268e7fa589

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

Copilot AI commented Apr 7, 2026

@copilot Subsystem tests are supposed to limit themselves to testing via subsystem functionality. Why is the Run() testing necessary? Is the testing at the system level not sufficient, or is there an ...

Agreed — the Configuration_Run_* tests were over-reach. Program.Run() is the Cli layer; the Configuration subsystem's own API is VersionMarkConfig.Load(), which is already fully tested by VersionMarkConfigLoadTests.cs. Every scenario covered by those Run() tests had a direct counterpart already present there (e.g. Configuration_Run_NonExistentFile_FailsVersionMarkConfig_Load_MissingFile_ReturnsNullConfig, etc.).

Fixed in commit 709442c:

  • Removed all 9 Configuration_Run_* tests from ConfigurationTests.cs
  • Updated load.yaml to point each tool-level requirement at the matching VersionMarkConfig_Load_* unit test from VersionMarkConfigLoadTests.cs

The --lint flag at the system level is already covered by Cli_Run_LintFlag_ValidConfig_Succeeds in CliTests.cs, so there is no gap.

@Malcolmnixon Malcolmnixon marked this pull request as ready for review April 7, 2026 01:16
Copilot AI review requested due to automatic review settings April 7, 2026 01:16
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

This PR aligns subsystem test suite naming with established conventions by removing the redundant Subsystem suffix from subsystem test file/class names, and updates all downstream references (reqstream + ReviewMark) to the renamed test methods/files.

Changes:

  • Renamed subsystem test suite files/classes and updated test method prefixes (e.g., CliSubsystem_*Cli_*).
  • Removed the ConfigurationLoadSubsystemTests CLI-layer test suite and ensured configuration load requirements point at VersionMarkConfig.Load unit tests instead.
  • Updated reqstream YAML test references and .reviewmark.yaml paths to match the new names.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/DemaConsulting.VersionMark.Tests/Cli/CliTests.cs Renames CLI subsystem test class and method prefixes to drop Subsystem.
test/DemaConsulting.VersionMark.Tests/Capture/CaptureTests.cs Renames Capture subsystem test class and method prefixes to drop Subsystem.
test/DemaConsulting.VersionMark.Tests/Publishing/PublishingTests.cs Renames Publishing subsystem test class and method prefixes to drop Subsystem.
test/DemaConsulting.VersionMark.Tests/SelfTest/SelfTestTests.cs Renames SelfTest subsystem test class and method prefixes to drop Subsystem.
test/DemaConsulting.VersionMark.Tests/Configuration/ConfigurationTests.cs Renames Configuration subsystem test class and method prefixes to drop Subsystem.
test/DemaConsulting.VersionMark.Tests/Configuration/ConfigurationLoadSubsystemTests.cs Removes CLI-layer “configuration load via Program.Run” subsystem tests (file deleted).
docs/reqstream/version-mark/cli/cli.yaml Updates requirement-to-test links to renamed Cli_* tests.
docs/reqstream/version-mark/capture/capture.yaml Updates requirement-to-test links to renamed Capture_* tests.
docs/reqstream/version-mark/publishing/publishing.yaml Updates requirement-to-test links to renamed Publishing_* tests.
docs/reqstream/version-mark/self-test/self-test.yaml Updates requirement-to-test links to renamed SelfTest_* tests.
docs/reqstream/version-mark/configuration/configuration.yaml Updates requirement-to-test links to renamed Configuration_* tests.
docs/reqstream/version-mark/configuration/load.yaml Repoints load requirements from removed CLI-layer tests to VersionMarkConfig_Load_* unit tests.
.reviewmark.yaml Updates review-set paths to renamed subsystem test files and removes the deleted load-subsystem test file reference.

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

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