Add Regression Coverage for the Two Install-Summary Lines

[ptr727]

CodeRabbit, reviewing the develop -> main promotion PR #982, correctly
flagged that #981/#983's two-line install-summary output had no
regression coverage: MainExitCodeCase only asserted the exit code.
Adds a case that captures stdout and asserts both lines are present
and separate.

Summary by CodeRabbit

• Tests
  • Added coverage verifying that skills materialization and marketplace registration results are displayed on separate output lines.

[qodo-code-review]

PR Summary by Qodo

Add regression test for two-line install summary stdout output

🧪 Tests 🕐 10-20 Minutes

AI Description

• Capture stdout from skills_install.main() to prevent install-summary formatting regressions
• Assert skills-materialized and marketplace-registered messages appear as separate output lines

Diagram

graph TD
  T["test_skills_install.py"] --> R["redirect_stdout"] --> M["skills_install.main()"] --> O[("stdout buffer")]
  O --> A["assert two lines"]

  subgraph Legend
    direction LR
    _t["Test"] ~~~ _fn["Function"] ~~~ _buf[("Buffer")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use pytest capsys/capfd capture (if pytest is in use)

• ➕ More idiomatic output capture for pytest-based suites
• ➕ Cleaner assertions without manual StringIO plumbing
• ➖ May not fit current unittest-style tests without refactoring
• ➖ Introduces fixture-style coupling if the suite isn’t pytest-native

2. Refactor main() to return a structured result (and test that)

• ➕ Avoids brittle stdout assertions
• ➕ Makes behavior easier to validate and reuse programmatically
• ➖ Larger production-code change for a formatting regression concern
• ➖ Could be overkill if stdout is the contract users rely on

Recommendation: Keep the current approach: capturing stdout with redirect_stdout is minimal, localized, and matches the regression risk (accidentally collapsing two prints into one). Consider a structured return value only if more output formatting contracts need frequent testing.

Files changed (1) +12 / -0

Tests (1) +12 / -0

test_skills_install.pyAdd stdout regression test for two install-summary lines +12/-0

Add stdout regression test for two install-summary lines

• Imports contextlib/io and adds a new test that captures stdout while running skills_install.main(). The test asserts both the skills materialization message and marketplace registration message are present as separate lines.

scripts/tests/test_skills_install.py

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 62299f17-0384-4cc9-a937-bacf4206f098

📥 Commits

Reviewing files that changed from the base of the PR and between b9bd726 and 45df682.

📒 Files selected for processing (1)

• scripts/tests/test_skills_install.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds output-capture imports and a test for skills_install.main(). The test verifies that skills materialization and marketplace registration results print on separate lines after successful execution.

Changes

Skills installation output

Layer / File(s)	Summary
Output separation regression test scripts/tests/test_skills_install.py	Adds contextlib and io imports. Adds a test that captures skills_install.main() output and checks separate skills and marketplace status lines.

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

Merge Risk: ⚪ Minimal · up to 45df6

This localized test-only change adds regression coverage for the two install-summary output lines; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: adding regression coverage for the two install-summary output lines.
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.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/947-stdout-test

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗