Skip to content

Conversation

@fredrikekelund
Copy link
Contributor

@fredrikekelund fredrikekelund commented Dec 1, 2025

Related issues

Proposed Changes

The tests in cli/commands/site/tests are all integration tests, which means some level of mocking is required, and we focus more on behavior than output. The goal is not mock-free unit tests, but I still found room for improvement in two areas:

  1. We consistently inspect logger output to verify function failures. This is not ideal, because logger output is a secondary effect of the failure.
  2. Especially for the more complex commands (like cli/commands/site/create.ts), we mock A LOT of stuff.

This PR addresses those concerns by:

  1. Moving the logger.reportError output to the yargs handlers in the registerCommand functions. This allows us to actually expect exceptions to be thrown from runCommand calls in the tests.
  2. Remove mock calls where possible. Notably, the logger isn't mocked anywhere in cli/commands/site/tests now. cli/commands/site/create.ts also reads the blueprint file and parses it as JSON in the yargs handler, reducing the amount of mocking needed in the tests.

Testing Instructions

Green unit tests and code review

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@fredrikekelund fredrikekelund requested review from a team and bcotrim December 1, 2025 11:00
@fredrikekelund fredrikekelund self-assigned this Dec 1, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2025

📊 Performance Test Results

Comparing 4cebaad vs trunk

site-editor

Metric trunk 4cebaad Diff Change
load 10132.00 ms 9773.00 ms -359.00 ms 🟢 -3.5%

site-startup

Metric trunk 4cebaad Diff Change
siteCreation 13331.00 ms 13176.00 ms -155.00 ms 🟢 -1.2%
siteStartup 5965.00 ms 5938.00 ms -27.00 ms 🟢 -0.5%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change

Copy link
Contributor

@bcotrim bcotrim left a comment

Choose a reason for hiding this comment

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

Amazing work, tests easier to read and write! 👍

Image

@fredrikekelund fredrikekelund merged commit f7e719d into trunk Dec 2, 2025
8 of 9 checks passed
@fredrikekelund fredrikekelund deleted the f26d/improve-cli-site-tests branch December 2, 2025 12:59
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