Skip to content

Conversation

@lalinsky
Copy link
Owner

@lalinsky lalinsky commented Sep 9, 2025

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 9, 2025

Walkthrough

Adds a new Bash script check.sh to run formatting and unit/e2e tests with CLI options and updates CLAUDE.md to document using ./check.sh (including --test-filter, --test-fail-first, --test-log-capture, --test-verbose, and --ci behavior).

Changes

Cohort / File(s) Summary
Docs: testing workflow update
CLAUDE.md
Replaced prior Zig-invocation guidance (zig build test, zig fmt, TEST_FILTER/TEST_LOG_CAPTURE notes) with instructions to use ./check.sh and documented options (--test-filter, --test-fail-first, --test-log-capture, --test-verbose, --ci) and recommended usage patterns.
Tooling: unified checks script
check.sh
New Bash script added. Implements strict shell options, parses CLI flags, exports test-related env vars, runs zig fmt (checks in CI, writes locally), executes zig build test-unit and zig build test-e2e, and prints status messages.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is currently missing or empty, offering no context or summary of the changes, which means it does not describe any aspect of the changeset. Please add a clear description summarizing the changes in this pull request, including the addition of check.sh and the updates to CLAUDE.md, so reviewers understand its purpose and scope.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Add check.sh” directly captures the primary change in this pull request—the introduction of a new check.sh script for orchestrating formatting and tests—and is concise and specific enough for a reviewer scanning the history to immediately grasp the main addition.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f97184 and 841b25c.

📒 Files selected for processing (1)
  • check.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • check.sh
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch check-sh

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.

Copy link
Contributor

@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: 2

🧹 Nitpick comments (4)
check.sh (3)

43-50: Surface tool versions in logs (helps CI debugging).

Print Zig version before formatting.

 echo "=== Formatting code ==="
+zig version || true
 if [ "$CI_MODE" = true ]; then

52-59: Avoid exporting empty TEST_FILTER; normalize booleans.

Minor: export a default empty filter explicitly and validate booleans to catch typos.

-# Set up environment variables for tests
-if [ -n "$TEST_FILTER" ]; then
-    export TEST_FILTER
-fi
-export TEST_LOG_CAPTURE="$TEST_LOG_CAPTURE"
-export TEST_FAIL_FIRST="$TEST_FAIL_FIRST" 
-export TEST_VERBOSE="$TEST_VERBOSE"
+# Set up environment variables for tests
+export TEST_FILTER="${TEST_FILTER:-}"
+case "$TEST_LOG_CAPTURE" in true|false) ;; * ) echo "Invalid --test-log-capture: $TEST_LOG_CAPTURE"; exit 1;; esac
+case "$TEST_FAIL_FIRST"  in true|false) ;; * ) echo "Invalid --test-fail-first: $TEST_FAIL_FIRST"; exit 1;; esac
+case "$TEST_VERBOSE"     in true|false) ;; * ) echo "Invalid --test-verbose: $TEST_VERBOSE"; exit 1;; esac
+export TEST_LOG_CAPTURE TEST_FAIL_FIRST TEST_VERBOSE

60-75: Optional: allow skipping unit or e2e to speed inner-loop.

Add --only-unit / --only-e2e toggles so contributors can iterate faster.

Happy to submit a follow-up PR wiring these flags.

CLAUDE.md (1)

3-7: Fix MD038 (spaces inside code spans) and document all supported flags.

Line 4 has a trailing space inside backticks; also the script supports --ci, --test-log-capture, and --test-verbose but they’re not mentioned here.

-- use `./check.sh --test-filter "test name" ` to run specific tests
+- use `./check.sh --test-filter "test name"` to run specific tests
+- optional flags: `--ci`, `--test-log-capture true|false`, `--test-verbose true|false`
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d353e0 and 0f97184.

📒 Files selected for processing (2)
  • CLAUDE.md (1 hunks)
  • check.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: lalinsky/nats.zig#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T10:22:22.247Z
Learning: Run `zig build test` to confirm your changes are working
Learnt from: CR
PR: lalinsky/nats.zig#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T10:22:22.247Z
Learning: Applies to **/*.zig : Format code using `zig fmt` before commit
📚 Learning: 2025-08-29T10:22:22.247Z
Learnt from: CR
PR: lalinsky/nats.zig#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T10:22:22.247Z
Learning: Run `zig build test` to confirm your changes are working

Applied to files:

  • CLAUDE.md
  • check.sh
📚 Learning: 2025-08-25T14:51:33.570Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-08-29T10:22:22.247Z
Learnt from: CR
PR: lalinsky/nats.zig#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T10:22:22.247Z
Learning: Applies to **/*.zig : Format code using `zig fmt` before commit

Applied to files:

  • CLAUDE.md
  • check.sh
📚 Learning: 2025-08-29T10:22:22.247Z
Learnt from: CR
PR: lalinsky/nats.zig#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T10:22:22.247Z
Learning: Applies to docs/reference/*.md : Read files in docs/reference/*.md for knowledge about the NATS protocol

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-08-30T21:03:44.011Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#67
File: src/dispatcher.zig:20-20
Timestamp: 2025-08-30T21:03:44.011Z
Learning: In the nats.zig project, test_runner.zig and files in the examples/ directory are fine to keep their std.debug.print calls - these don't need to be replaced with the centralized logging system.

Applied to files:

  • CLAUDE.md
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md

4-4: Spaces inside code span elements

(MD038, no-space-in-code)

- Add set -euo pipefail and IFS for safer script execution
- Add argument validation for flags that require values
- Add -h/--help flag with usage information
- Prevent crashes when required arguments are missing

Co-authored-by: Lukáš Lalinský <[email protected]>
@lalinsky lalinsky merged commit be22829 into main Sep 9, 2025
3 checks passed
@lalinsky lalinsky deleted the check-sh branch September 9, 2025 20:58
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.

1 participant