Skip to content

Fail the build on failed HQRM blocks and containers#179

Merged
johlju merged 1 commit into
dsccommunity:mainfrom
nohwnd:fix/hqrm-gate-on-failed-containers
Jul 2, 2026
Merged

Fail the build on failed HQRM blocks and containers#179
johlju merged 1 commit into
dsccommunity:mainfrom
nohwnd:fix/hqrm-gate-on-failed-containers

Conversation

@nohwnd

@nohwnd nohwnd commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Pull Request (PR) description

Fail_Build_If_HQRM_Tests_Failed gated the build on FailedCount only:

Assert-Build -Condition ($DscTestObject.FailedCount -eq 0) -Message ('Failed {0} tests. Aborting Build.' -f $DscTestObject.FailedCount)

FailedCount only counts failed tests. A Pester 5 discovery/container failure — for example a Describe/Context with an empty or $null -ForEach, or a parse error in a test file — fails during discovery and never runs a test. It sets FailedContainersCount/FailedBlocksCount and marks the run Result as Failed, but leaves FailedCount at 0. The gate therefore passed and the build went green even though HQRM testing had actually failed. (This is the same class of failure that recently slipped through in dbachecks.)

This PR gates on the run Result instead, which already accounts for failed tests, blocks and containers:

Assert-Build -Condition ($DscTestObject.Result -eq 'Passed') -Message $assertMessage

This mirrors the container-aware check already present in Invoke_HQRM_Tests.build.ps1 (if ($script:testResults.Result -eq 'Failed') { throw ... }); the separate Fail_Build_If_HQRM_Tests_Failed gate task simply had not been updated to match. The repository already requires Pester -MinimumVersion 5.1, so Result is always present and no Pester 4 fallback is needed. The assertion message now also reports the failed block and container counts to make such failures easier to diagnose.

This Pull Request (PR) fixes the following issues

None

Task list

  • Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
  • Documentation added/updated in README.md.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

Fail_Build_If_HQRM_Tests_Failed gated only on FailedCount, but a Pester 5
discovery/container failure (for example an empty -ForEach) does not increase
FailedCount. It sets FailedContainersCount/FailedBlocksCount and marks the run
Result as 'Failed', so the build wrongly passed as green.

Gate on the Result property, which already accounts for failed tests, blocks
and containers. This matches the container-aware check already used in the
Invoke_HQRM_Tests task.

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

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The Fail_Build_If_HQRM_Tests_Failed build task now gates build failure on the Pester Result property rather than solely FailedCount, catching discovery/container/parse failures that don't increment FailedCount. The changelog is updated to document this behavior change.

Changes

Pester Result-based build gating

Layer / File(s) Summary
Result-based gating check
source/tasks/Fail_Build_If_HQRM_Tests_Failed.build.ps1
Replaces the FailedCount-only assertion with a check on $DscTestObject.Result -eq 'Passed', adding a formatted message reporting failed tests, failed blocks, and failed containers, plus a comment explaining why FailedCount alone is insufficient.
Changelog update
CHANGELOG.md
Documents the new failure-gating behavior based on Pester Result under Unreleased/Fixed.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: failing HQRM builds on failed blocks and containers.
Description check ✅ Passed The description clearly matches the changeset and explains the updated Pester gating logic and changelog entry.
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.

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
source/tasks/Fail_Build_If_HQRM_Tests_Failed.build.ps1 (1)

109-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Line exceeds 120-character guideline.

Line 109 is roughly 131 characters. Consider breaking the message string across lines.

As per path instructions, PowerShell files should "Try to limit lines to 120 characters".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@source/tasks/Fail_Build_If_HQRM_Tests_Failed.build.ps1` around lines 109 -
110, The $assertMessage assignment in Fail_Build_If_HQRM_Tests_Failed.build.ps1
exceeds the 120-character guideline; split the long formatted string across
multiple lines while keeping the same Pester failure message and formatting
logic in the $assertMessage construction.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@source/tasks/Fail_Build_If_HQRM_Tests_Failed.build.ps1`:
- Around line 109-110: The $assertMessage assignment in
Fail_Build_If_HQRM_Tests_Failed.build.ps1 exceeds the 120-character guideline;
split the long formatted string across multiple lines while keeping the same
Pester failure message and formatting logic in the $assertMessage construction.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 746fabb7-59ab-40ce-a3a5-31d6c6b6aa56

📥 Commits

Reviewing files that changed from the base of the PR and between 5efd59f and fd258bd.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • source/tasks/Fail_Build_If_HQRM_Tests_Failed.build.ps1

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79%. Comparing base (5efd59f) to head (fd258bd).

Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #179   +/-   ##
===================================
  Coverage    79%    79%           
===================================
  Files        43     43           
  Lines       569    569           
===================================
  Hits        452    452           
  Misses      117    117           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@johlju johlju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:lgtm:

@johlju reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on nohwnd).

@johlju

johlju commented Jul 2, 2026

Copy link
Copy Markdown
Member

@dan-hughes fyi I push this in a full release shortly, if there are any problems out there you know why 🙂

@johlju johlju merged commit 4b8d410 into dsccommunity:main Jul 2, 2026
12 checks passed
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