Skip to content

fix(tests): parse changelog version with Select-String instead of break in ForEach-Object#37

Open
tablackburn wants to merge 2 commits into
mainfrom
fix/changelog-parse-select-string
Open

fix(tests): parse changelog version with Select-String instead of break in ForEach-Object#37
tablackburn wants to merge 2 commits into
mainfrom
fix/changelog-parse-select-string

Conversation

@tablackburn
Copy link
Copy Markdown
Owner

@tablackburn tablackburn commented May 27, 2026

Summary

Replaces the changelog-version parse in tests/Manifest.tests.ps1 with Select-String, removing a latent break-in-ForEach-Object bug.

BeforeAll parsed the module version from CHANGELOG.md with:

$changelogVersion = Get-Content $changelogPath | ForEach-Object {
    if ($_ -match $changelogVersionPattern) { $changelogVersion = $matches.Version; break }
}

break inside a ForEach-Object script block is unreliable: a pipeline is not a loop, so break looks for an enclosing loop and, finding none, terminates the surrounding block unexpectedly rather than cleanly stopping the pipeline at the first match. It also assigns $changelogVersion twice (inside the block, then again to the pipeline's output).

The replacement pulls the first matching line's named capture group directly — no loop, no break:

$changelogVersion = (Select-String -Path $changelogPath -Pattern $changelogVersionPattern |
    Select-Object -First 1).Matches[0].Groups['Version'].Value

Validation

The raw template isn't Pester-tested locally or in CI (the is_template gate in CI.yaml skips Build/Test while {{GUID}} etc. are still placeholders — the GENERATEMARKDOWN/manifest-import step can't parse them). So this is validated the way template scaffolding always is — downstream:

No CHANGELOG entry, consistent with #35 (a prior test-internal fix that didn't add one).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Improved changelog version extraction test for better performance and robustness; it now handles missing version entries cleanly without unnecessary iteration.

Review Change Stack

…ak in ForEach-Object

The BeforeAll changelog-version parse in tests/Manifest.tests.ps1 used
`Get-Content $changelogPath | ForEach-Object { ... break }`. `break` inside
ForEach-Object is unreliable: a pipeline is not a loop, so `break` targets an
enclosing loop if one exists, or otherwise terminates the block unexpectedly,
rather than cleanly stopping at the first match. Replace it with Select-String,
which returns the first matching line's named capture group directly — no loop
and no break.

Behavior is unchanged (still resolves to the topmost `## [Version]` entry).
Surfaced while aligning a downstream module's Manifest test
(tablackburn/ReScenePS#22).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings May 27, 2026 05:20
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2f443998-0f75-4b22-93c7-603a88995ad0

📥 Commits

Reviewing files that changed from the base of the PR and between 6ed0a7f and 4497839.

📒 Files selected for processing (1)
  • tests/Manifest.tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/Manifest.tests.ps1

📝 Walkthrough

Walkthrough

Refactors changelog version extraction in tests/Manifest.tests.ps1 from line-by-line Get-Content iteration to Select-String with Select-Object -First 1, assigning the Version named capture or $null if missing.

Changes

Changelog Version Extraction Refactoring

Layer / File(s) Summary
Changelog version extraction with Select-String
tests/Manifest.tests.ps1
Changelog version extraction is refactored from Get-Content piped into ForEach-Object with manual line matching and break to Select-String selecting the first match and directly accessing the named capture group Version, falling back to $null if no match.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐰 In CHANGELOG fields I used to peep and peep,
Now Select-String finds the version in a single leap,
A named capture caught, no looping fuss,
Clean lines, quick hops — that's enough for us!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: replacing break-in-ForEach-Object pattern with Select-String for changelog version parsing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/changelog-parse-select-string

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

tablackburn added a commit to tablackburn/ReScenePS that referenced this pull request May 27, 2026
… foreach

Replace the foreach-over-Get-Content changelog-version parse with Select-String,
which returns the first matching line's named capture group directly — no loop and
no break. The foreach existed only to avoid the canonical template's unreliable
`ForEach-Object { ... break }`; Select-String removes the need for either form.
Mirrors the upstream fix in tablackburn/PowerShellModuleTemplate#37.

Build + Pester pass locally: 786 passed, 0 failed, 23 skipped.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Copy link
Copy Markdown

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 updates the module-manifest Pester tests to parse the changelog version using Select-String, avoiding the unreliable use of break inside a ForEach-Object pipeline in BeforeAll.

Changes:

  • Replaced Get-Content | ForEach-Object { ... break } changelog parsing with a Select-String-based approach.
  • Added an explanatory comment describing why break in this context is unreliable.

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

Comment thread tests/Manifest.tests.ps1 Outdated
Capture the Select-String MatchInfo before indexing so a missing or malformed changelog heading leaves $changelogVersion as $null and fails the assertion cleanly, instead of throwing "Cannot index into a null array" in BeforeAll.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants