Skip to content

Conversation

@nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Dec 17, 2025

PR Description

Enable execute tests to run on Windows

This PR makes the execute tests cross-platform by:

  • Removing the @TestOn('!windows') annotation that excluded all tests from Windows
  • Adding new tests for working directory, stderr streaming, and complex shell commands that work on both platforms
  • Skipping only the SIGINT forwarding test on Windows (bash trap has no Windows equivalent, and Process.kill(ProcessSignal.sigint) doesn't trigger signal handlers on Windows anyway)
  • Fixing echo "Hello world!" to echo Hello world! for cross-platform compatibility
  • Implementing MockStdout.addStream() to support the new tests

Summary by CodeRabbit

  • Public API

    • Execution helper now accepts working directory and custom stdout/stderr sinks.
  • Tests

    • Expanded coverage for command execution: working-directory runs, stderr streaming, complex shell commands, and platform-specific error messaging.
    • Adjusted SIGINT test behavior with Windows-specific skip reason.
  • Chores

    • Improved test utility stream handling.
    • CI expanded to run tests on Ubuntu, macOS, and Windows.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 17, 2025

Warning

Rate limit exceeded

@nielsenko has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 12 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 18ef1df and 8dff006.

📒 Files selected for processing (3)
  • .github/workflows/ci.yml (1 hunks)
  • packages/cli_tools/test/execute_test.dart (3 hunks)
  • packages/cli_tools/test/test_utils/mock_stdout.dart (1 hunks)
📝 Walkthrough

Walkthrough

Refactors and expands tests for the execute helper (adds workingDirectory, stdout, stderr usage; stderr streaming; complex shell command; SIGINT handling) and implements MockStdout.addStream; also restructures CI workflow YAML and expands job matrices.

Changes

Cohort / File(s) Summary
Test execution enhancements
packages/cli_tools/test/execute_test.dart
Added imports (test_descriptor, test_utils/mock_stdout), removed quotes from an echoed test string, refactored SIGINT test formatting and added a Windows skip, and added tests for running with a specified working directory, streaming stderr, and handling complex shell commands; tests now call execute(...) with workingDirectory, stdout, and stderr.
Mock stdout utility
packages/cli_tools/test/test_utils/mock_stdout.dart
Implemented addStream to consume incoming byte chunks by calling add for each element and return a Future that completes when the stream is processed (replacing a previous UnimplementedError).
CI workflow
.github/workflows/ci.yml
Reworked YAML job structure/indentation and expanded matrices to include platform axis (ubuntu/windows/macos) and explicit Dart SDK matrix entries; changes are structural/formatting for CI matrix coverage.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to cross-platform differences in the new tests (Windows-specific skips and error message assertions).
  • Verify MockStdout.addStream correctly handles partial chunk boundaries and completes the returned Future when the stream ends.
  • Confirm CI matrix changes correctly map matrix variables (platform/dart/package) to job steps.

Possibly related PRs

Suggested reviewers

  • Isakdl
  • christerswahn

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: extending execute tests to work on Windows platform, matching the PR's core objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

@nielsenko nielsenko self-assigned this Dec 17, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)

32-52: LGTM! Consider selective multi-platform testing for optimization.

The addition of the platform matrix successfully extends testing to Windows and macOS, which aligns with the PR objective. All commands used are cross-platform compatible.

However, the platform matrix now applies to both cli_tools and config packages, increasing CI job count from 4 to 12 (3× execution time). If the config package doesn't contain platform-specific code, consider using a conditional matrix to test only cli_tools on multiple platforms while keeping config on ubuntu-latest only.

Example optimization (if config is platform-agnostic):

matrix:
  dart: [3.3, 3.9]
  package: [cli_tools, config]
  platform: [ubuntu-latest]
  include:
    - package: cli_tools
      platform: windows-latest
      dart: 3.3
    - package: cli_tools
      platform: windows-latest
      dart: 3.9
    - package: cli_tools
      platform: macos-latest
      dart: 3.3
    - package: cli_tools
      platform: macos-latest
      dart: 3.9
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between be6c019 and ac5ffb9.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml (1 hunks)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)

3-9: LGTM!

The workflow triggers are properly structured with standard YAML indentation.


12-30: LGTM!

The dart_format job is well-structured. Running format checks only on ubuntu-latest is appropriate since formatting results are platform-independent.

Copy link
Collaborator

@christerswahn christerswahn left a comment

Choose a reason for hiding this comment

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

LGTM

@nielsenko nielsenko merged commit 24b7d14 into serverpod:main Dec 18, 2025
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