Skip to content

Implement custom shell command tests and pipeline stages #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 45 commits into from
Apr 30, 2025

Conversation

ryan-gang
Copy link
Collaborator

@ryan-gang ryan-gang commented Apr 23, 2025

Introduce test functions for custom shell commands, enhance file writing logic, and add pipeline stages for testing. These changes improve the testing framework for shell utilities.

Summary by CodeRabbit

  • New Features
    • Introduced a custom grep command-line utility with standard input support and pattern matching.
    • Added comprehensive system and pipeline tests for the new grep utility and related shell commands.
    • Added multi-stage pipeline test groups and new test targets for bash environment.
    • Added support for additional custom commands: head, tail, wc, and yes.
    • Added new language "gleam" and new challenge extensions for programmable completions and pipelines with multiple stages.
  • Bug Fixes
    • Improved assertion handling to gracefully skip empty expected outputs.
    • Enhanced error checking for pipe closure in tail command tests.
    • Preserved original line endings in head command output.
  • Chores
    • Updated .gitignore to exclude .devcontainer/.
    • Enhanced documentation comments and cleaned up whitespace in configuration files.
  • Tests
    • Added new test cases and helpers to validate grep and shell pipeline behaviors.
    • Added staged shell test scripts for bash and ash pipelines.

@ryan-gang ryan-gang self-assigned this Apr 23, 2025
Copy link

coderabbitai bot commented Apr 23, 2025

Walkthrough

This update introduces a new custom grep executable along with a comprehensive Go test suite for it. The Makefile is enhanced to build and test this new executable across OS and architecture combinations, integrating it into aggregate test targets and pipeline stages. Three new pipeline test functions (testP1, testP2, testP3) are added to the internal Go codebase, with corresponding test cases appended to the tester definition. Supporting utilities are extended with new constants, file append functionality, and refined assertion logic. The course definition YAML adds new language support and pipeline/completion stages. New shell test fixtures and pipeline stage tests for bash and ash shells are included. Minor improvements and cleanup are applied to existing code and test helpers.

Changes

File(s) Change Summary
Makefile Adds build and test targets for custom grep; defines new pipeline stages; updates aggregate test targets; adds new test target for pipeline stages with bash.
internal/custom_executable/grep/grep.go
internal/custom_executable/grep/grep_test.go
Adds a new custom grep executable implementation and a Go test suite supporting both custom and system grep binaries.
internal/custom_executable/build/grep.go Adds function to create the custom grep executable for current OS and architecture.
internal/stage_p1.go
internal/stage_p2.go
internal/stage_p3.go
Adds three new test functions (testP1, testP2, testP3) implementing pipeline and shell command test scenarios.
internal/tester_definition.go Appends three new test cases with slugs "br6", "ny9", and "xk3" for the new pipeline test functions.
internal/utils.go Adds constants for new custom commands (head, tail, wc, yes); reorders existing constants.
internal/utils_build.go Extends SetUpCustomCommands to handle new commands; updates comment on commands with no metadata; adds panic on unknown commands.
internal/test_helpers/course_definition.yml Adds language "gleam"; adds two new extensions ("completions" and "pipelines") with multiple new stages; updates existing stages with clarifications; removes trailing spaces in code blocks.
internal/assertions/single_line_assertion.go Updates panic condition to require both empty expected output and empty fallback patterns to panic.
internal/test_cases/command_with_multiline_response_test_case.go Adds SkipPromptAssertion field and logic to optionally skip prompt assertion in multiline response tests.
internal/utils_fs.go Renames parameter in writeFile; adds new appendFile function for appending content to files.
.gitignore Removes trailing blank line before .devcontainer entry; .devcontainer/ remains ignored.
internal/test_helpers/fixtures/bash/pipelines/pass Adds new bash pipeline test fixture script with staged shell command tests and output verifications.
internal/test_helpers/fixtures/ash/pipelines/pass Adds new ash pipeline test fixture script with staged shell command tests and output verifications.
internal/stages_test.go Adds new test cases for pipeline stages with bash and ash shells; adds regex normalization for ls -la output lines.
internal/custom_executable/head/head.go Modifies line reading in head command to preserve original line endings exactly when printing first N lines.
internal/custom_executable/tail/tail_test.go Updates error checks to use errors.Is for io.ErrClosedPipe in tail follow test.
internal/test_cases/command_multiple_completions_test_case.go Simplifies boolean expression for bell ringing condition without changing logic.

Sequence Diagram(s)

sequenceDiagram
    participant Make as Makefile
    participant Tester as Test Suite
    participant Grep as Custom Grep Executable
    participant SystemGrep as System Grep
    participant Shell as Shell Executable

    %% Build and Test Grep Executable
    Make->Grep: Build custom grep for all OS/arch
    Make->Tester: Run grep test suite
    Tester->Grep: Execute grep with stdin and pattern
    Grep-->>Tester: Return stdout, stderr, exit code

    %% Run Pipeline Stage Tests
    Make->Shell: Build shell and custom commands
    Make->Tester: Run pipeline stage tests (testP1, testP2, testP3)
    Tester->Shell: Execute shell commands with pipelines (cat, head, tail, wc, grep)
    Shell-->>Tester: Return outputs for assertions
Loading

Suggested reviewers

  • rohitpaulk

Poem

In the warren where code bunnies leap,
A new grep hops in, its patterns to keep.
Pipelines now flow with head, tail, and wc,
Tests multiply swiftly—oh, what a spree!
With every new stage, our shell friends delight,
For rabbits in code always get grep just right.
🐇✨


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 3

🧹 Nitpick comments (2)
Makefile (1)

212-214: Test target for pipeline stages added correctly.

The new test_pipeline_w_bash target properly runs the pipeline tests in a bash environment, following the same pattern as other test targets.

Consider also updating the main test_bash target to include the new pipeline tests for completeness.

test_bash:
	make test_base_w_bash
	make test_nav_w_bash
	make test_quoting_w_bash
	make test_redirections_w_bash
	make test_completions_w_bash
+	make test_pipeline_w_bash
internal/stage_p2.go (1)

30-31: Consider adding test documentation

The test inputs and expected outputs are defined clearly, but consider adding comments explaining the purpose of each test case to improve maintainability.

-	inputs := []string{`yes | head -n 1`, `yes | head -n 10000 | wc`}
-	expectedOutputs := []string{"y", fmt.Sprintf("%7d%8d%8d", 10000, 10000, 20000)}
+	// Test cases for pipeline command functionality
+	inputs := []string{
+		`yes | head -n 1`,      // Basic pipeline test - limit yes output to single line
+		`yes | head -n 10000 | wc`  // Multi-stage pipeline test with larger output
+	}
+	expectedOutputs := []string{
+		"y", 
+		fmt.Sprintf("%7d%8d%8d", 10000, 10000, 20000)  // Format: lines, words, bytes
+	}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4fe92bf and 8fea7df.

📒 Files selected for processing (7)
  • Makefile (3 hunks)
  • internal/stage_p1.go (1 hunks)
  • internal/stage_p2.go (1 hunks)
  • internal/stage_p3.go (1 hunks)
  • internal/tester_definition.go (1 hunks)
  • internal/utils.go (1 hunks)
  • internal/utils_build.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/stage_p1.go (4)
internal/utils_build.go (2)
  • SetUpCustomCommands (27-68)
  • CommandDetails (16-25)
internal/utils.go (2)
  • CUSTOM_CAT_COMMAND (14-14)
  • CUSTOM_WC_COMMAND (18-18)
internal/utils_fs.go (1)
  • GetShortRandomDirectory (53-79)
internal/test_cases/command_response_test_case.go (1)
  • CommandResponseTestCase (18-30)
internal/stage_p2.go (5)
internal/shell_executable/shell_executable.go (1)
  • NewShellExecutable (42-55)
internal/utils_build.go (2)
  • SetUpCustomCommands (27-68)
  • CommandDetails (16-25)
internal/utils.go (3)
  • CUSTOM_HEAD_COMMAND (15-15)
  • CUSTOM_WC_COMMAND (18-18)
  • CUSTOM_YES_COMMAND (19-19)
internal/logged_shell_asserter/logged_shell_asserter.go (1)
  • NewLoggedShellAsserter (25-37)
internal/test_cases/command_response_test_case.go (1)
  • CommandResponseTestCase (18-30)
🔇 Additional comments (11)
internal/utils.go (1)

15-19: New command constants added correctly.

The new constants for head, tail, wc, and yes commands follow the existing naming convention and support the newly added pipeline tests.

internal/utils_build.go (1)

23-23: Comment updated to include new commands.

The comment has been appropriately updated to include the new custom commands (Head, Tail, Wc, Yes) that don't require metadata, which aligns with the constants added in utils.go.

internal/tester_definition.go (1)

153-167:

✅ Verification successful

Test cases for pipeline stages added correctly.

The three new test cases for pipeline stages (p1, p2, p3) are correctly added to the test definition with the standard 15-second timeout, matching the pattern of existing test cases.

However, I'm not seeing the implementation files for testP1, testP2, and testP3 functions in the PR. Ensure that these functions are properly implemented in their respective files (likely stage_p1.go, stage_p2.go, and stage_p3.go).


🏁 Script executed:

#!/bin/bash
# Verify that the pipeline test implementation files exist
echo "Checking for pipeline stage implementation files..."
ls -la internal/stage_p*.go 2>/dev/null || echo "Pipeline stage implementation files not found!"

Length of output: 394


Implementation files for pipeline test functions present

The implementation files internal/stage_p1.go, internal/stage_p2.go, and internal/stage_p3.go are present and provide the definitions for testP1testP2, and testP3. All new test cases have corresponding implementations and match the existing patterns. Ready to merge.

Makefile (2)

148-154: Pipeline stages definition added correctly.

The new _PIPELINE_STAGES definition correctly defines three pipeline-related stages with appropriate slugs matching the test cases added in tester_definition.go.


180-180: Pipeline stages variable processed correctly.

The PIPELINE_STAGES variable is appropriately initialized using the quote_strings helper function, consistent with how other stage variables are processed.

internal/stage_p2.go (1)

33-43: LGTM: Test case implementation is clean and handles errors well

The loop implementation for running test cases is well-structured with proper error handling.

internal/stage_p3.go (3)

48-55: Test case implementation is robust with multiline assertion

Using MultiLineAssertion for validating the output is appropriate for this test case, as it handles line-by-line comparison of the output.


42-43:

✅ Verification successful

Consider potential timing issues with tail -f

The command tail -f is designed to continuously monitor a file and output new lines as they appear. This could potentially cause the test to hang or behave unpredictably if not handled properly.


🏁 Script executed:

#!/bin/bash
# Check how tail -f is handled - looking for potential timeout or process termination mechanisms
grep -n "tail -f" internal/
grep -n "MultilineResponseTestCase" internal/test_cases/

Length of output: 161


No hang: head -n 5 cleanly terminates the pipeline

The test uses:

tail -f <filePath> | head -n 5

Once head reads five lines it exits, sending SIGPIPE to tail, which then also terminates. No additional timeout or process‐killing logic is required.


19-22:

❓ Verification inconclusive

Verify that custom command setup works for head and tail

Similar to stage_p2.go, ensure that the SetUpCustomCommands function properly supports the "head" and "tail" command types.


🏁 Script executed:

#!/bin/bash
# Check if the command constants are properly defined
grep -n "CUSTOM_.*_COMMAND" internal/utils.go

Length of output: 263


#!/bin/bash
# Locate and display the implementation of SetUpCustomCommands to verify handling of CommandType values
set -e

# Find the file containing the function definition
file=$(rg -l "func SetUpCustomCommands" -n internal | head -1)
echo "==> Found in: $file"
echo "========================================="
# Print the function implementation (first 200 lines for context)
sed -n '1,200p' "$file"

#!/bin/bash
set -e

# Locate the file containing SetUpCustomCommands
file=$(rg -l "func SetUpCustomCommands" -n internal | head -1)
echo "==> Implementation found in: $file"
echo "============================================================"

# Print the function implementation (first 200 lines for context)
sed -n '1,200p' "$file"

echo
echo "=== Checking for explicit handling of \"head\" and \"tail\" ==="
grep -R -n "case \"head\"" -n "$file" || echo "No explicit \"head\" case found"
grep -R -n "case \"tail\"" -n "$file" || echo "No explicit \"tail\" case found"

#!/bin/bash
set -e

# Locate the file containing SetUpCustomCommands
file=$(rg -l --glob 'internal/**/*.go' 'func SetUpCustomCommands' | head -1)
echo "==> Implementation found in: $file"
echo "--------------------------------------------------"

# Show the implementation (first 200 lines for context)
sed -n '1,200p' "$file"
echo
echo "==> Checking for explicit handling of \"head\" and \"tail\" in switch/case:"
rg -n 'case\s*"(head|tail)"' "$file" || echo "No explicit case for head or tail found"

#!/bin/bash
set -e

# Locate the SetUpCustomCommands function definition
file=$(grep -R -n "func SetUpCustomCommands" internal | head -1 | cut -d: -f1)
echo "==> Definition found in: $file"
echo "--------------------------------------------------"

# Display the first 200 lines of the file for context
sed -n '1,200p' "$file"

echo
echo "==> Verifying explicit handling of \"head\" and \"tail\":"
grep -nE 'case\s*"(head|tail)"' "$file" || echo "No explicit case for head or tail found"

#!/bin/bash
set -e

# Locate the implementation of SetUpCustomCommands
file=$(rg -l "func SetUpCustomCommands" --glob "internal/**/*.go" | head -n1)
echo "==> Implementation found in: $file"
echo "--------------------------------------------------"

# Display the function body
awk '/func SetUpCustomCommands/,/^\}/' "$file"

echo
echo "==> Checking for explicit handling of \"head\" and \"tail\":"
grep -nE 'case\s*"(head|tail)"' "$file" || echo "No explicit case for \"head\" or \"tail\" found"

Verify custom command support for “head” and “tail” in SetUpCustomCommands

Please confirm that the SetUpCustomCommands implementation explicitly handles the "head" and "tail" command types—similar to how it handles other commands—so that invoking:

SetUpCustomCommands(stageHarness, shell, []CommandDetails{
  {CommandType: "head", CommandName: CUSTOM_HEAD_COMMAND, CommandMetadata: ""},
  {CommandType: "tail", CommandName: CUSTOM_TAIL_COMMAND, CommandMetadata: ""},
}, false)

actually wires up the custom head/tail binaries. In particular, check:

  • The source file defining func SetUpCustomCommands (e.g. under internal/…) contains a case "head" and case "tail" (or equivalent mapping).
  • It uses the constants from internal/utils.go (lines 15–17) for dispatch.
internal/stage_p1.go (2)

52-56: LGTM: Test case implementation is clean

The test case structure is well-defined with appropriate fields for command, expected output, and success message.


37-39:

❓ Verification inconclusive

Verify that the word count logic matches wc implementation

The manual calculation of lines, words, and bytes might not exactly match what the wc command outputs depending on how it handles newlines and whitespace. Consider validating this logic or documenting the expected differences.


🏁 Script executed:

#!/bin/bash
# Check if there are any tests specifically for the wc command implementation
grep -n "wc" internal/custom_executable/

Length of output: 90


Verify word‑count logic and add tests
I didn’t find any existing tests that compare your manual line/word/byte counts against the standard wc behavior. Please ensure the implementation in:

  • File: internal/stage_p1.go
    Lines 37–39
    lines := strings.Count(fileContent, "\n")
    words := strings.Count(strings.ReplaceAll(fileContent, "\n", " "), " ") + 1
    bytes := len(fileContent)

matches the output of wc -l, wc -w, and wc -c (especially around trailing newlines and multiple whitespace).
• Add tests that feed representative inputs into this function and assert equality with wc.
• If there are known, intentional differences from wc, document them clearly.

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: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 8fea7df and 467c5d6.

📒 Files selected for processing (1)
  • internal/test_helpers/course_definition.yml (6 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
internal/test_helpers/course_definition.yml

[error] 941-941: no new line character at the end of file

(new-line-at-end-of-file)

🔇 Additional comments (6)
internal/test_helpers/course_definition.yml (6)

558-558: Remove trailing spaces in single-quoted cat example
The trailing spaces after the file paths in this code block were cleaned up, which improves consistency and prevents accidental formatting in the rendered markdown.


599-599: Remove trailing spaces in double-quoted cat example
Consistently removing trailing whitespace from the code block keeps the examples tidy and avoids unintended line breaks.


640-640: Remove trailing spaces in backslash-escaped cat example
Trailing whitespace was removed to keep the code snippet clean and uniform across all quoting examples.


681-681: Remove trailing spaces in single-quote/backslash cat example
Cleaning up trailing spaces in this snippet ensures consistency with the other quoting examples.


722-722: Remove trailing spaces in mixed-quote cat example
The example demonstrating backslashes within double-quoted segments has had its trailing whitespace removed for consistency.


939-941: Add new pipeline stage slugs
The slugs p1, p2, and p3 have been appended to the stages list to reflect the newly implemented testP1, testP2, and testP3 stages. Please verify that these slugs are correctly registered in the tester definitions (internal/tester_definition.go) and that the Makefile target test_pipeline_w_bash invokes them as intended.

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 941-941: no new line character at the end of file

(new-line-at-end-of-file)

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

🧹 Nitpick comments (6)
internal/stage_p5.go (1)

26-34: Minor shell pipeline sanity check

The command under test is ls | type exit, but only the second part produces output.
If the goal is to validate pipe handling, consider asserting on stdout of both sides (e.g. check that no unexpected ls listing sneaks through).

internal/stage_p3.go (1)

48-50: Redundant second write to the same file

Immediately after computing expectedOutput the code rewrites the identical fileContent to disk:

if err := writeFiles([]string{filePath}, []string{fileContent}, logger); err != nil { ... }

This second write is unnecessary and can be removed to save I/O and log noise.

internal/stage_p6.go (1)

45-46: Consider improving word count calculation to handle more complex content.

The current word counting logic assumes each word is separated by a single space after replacing newlines. This might not accurately count words if the random words themselves contain spaces.

-words := strings.Count(strings.ReplaceAll(fileContent, "\n", " "), " ") + 1
+// Split by whitespace to count words more accurately
+words := len(strings.Fields(strings.ReplaceAll(fileContent, "\n", " ")))
internal/test_cases/command_with_multiline_response_test_case.go (3)

22-23: Fix comment-field name mismatch.

The field comment uses SkipAssertPrompt while the actual field is named SkipPromptAssertion.

-// SkipAssertPrompt is a flag to indicate that the prompt should not be asserted
+// SkipPromptAssertion is a flag to indicate that the prompt should not be asserted

38-46: Simplify error handling logic.

The conditional structure has duplicate error handling logic that can be simplified.

-if !t.SkipPromptAssertion {
-   if err := asserter.AssertWithPrompt(); err != nil {
-       return err
-   }
-} else {
-   if err := asserter.AssertWithoutPrompt(); err != nil {
-       return err
-   }
-}
+var err error
+if !t.SkipPromptAssertion {
+   err = asserter.AssertWithPrompt()
+} else {
+   err = asserter.AssertWithoutPrompt()
+}
+if err != nil {
+   return err
+}

22-24: Improve documentation for the new field.

The current field comment doesn't explain when you would use this flag or the implications of skipping prompt assertion.

-// SkipAssertPrompt is a flag to indicate that the prompt should not be asserted
+// SkipPromptAssertion controls whether to expect a shell prompt after command execution.
+// Set to true when testing commands with continuous output or when the command doesn't return
+// to a prompt state (e.g., long-running processes, interactive commands).
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 467c5d6 and 46377a5.

📒 Files selected for processing (7)
  • internal/assertions/single_line_assertion.go (1 hunks)
  • internal/stage_p3.go (1 hunks)
  • internal/stage_p4.go (1 hunks)
  • internal/stage_p5.go (1 hunks)
  • internal/stage_p6.go (1 hunks)
  • internal/test_cases/command_with_multiline_response_test_case.go (2 hunks)
  • internal/utils_fs.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/stage_p6.go (6)
internal/shell_executable/shell_executable.go (1)
  • NewShellExecutable (42-55)
internal/utils_build.go (2)
  • SetUpCustomCommands (27-68)
  • CommandDetails (16-25)
internal/utils.go (3)
  • CUSTOM_CAT_COMMAND (14-14)
  • CUSTOM_HEAD_COMMAND (15-15)
  • CUSTOM_WC_COMMAND (18-18)
internal/logged_shell_asserter/logged_shell_asserter.go (1)
  • NewLoggedShellAsserter (25-37)
internal/utils_fs.go (1)
  • GetShortRandomDirectory (53-79)
internal/test_cases/command_response_test_case.go (1)
  • CommandResponseTestCase (18-30)
internal/stage_p4.go (5)
internal/shell_executable/shell_executable.go (1)
  • NewShellExecutable (42-55)
internal/utils_build.go (2)
  • SetUpCustomCommands (27-68)
  • CommandDetails (16-25)
internal/utils.go (1)
  • CUSTOM_WC_COMMAND (18-18)
internal/logged_shell_asserter/logged_shell_asserter.go (1)
  • NewLoggedShellAsserter (25-37)
internal/test_cases/command_response_test_case.go (1)
  • CommandResponseTestCase (18-30)
🪛 golangci-lint (1.64.8)
internal/stage_p6.go

15-15: func testP6 is unused

(unused)

internal/stage_p5.go

10-10: func testP5 is unused

(unused)

internal/stage_p4.go

14-14: func testP4 is unused

(unused)

🪛 GitHub Check: lint
internal/stage_p6.go

[failure] 15-15:
func testP6 is unused (U1000)

internal/stage_p5.go

[failure] 10-10:
func testP5 is unused (U1000)

internal/stage_p4.go

[failure] 14-14:
func testP4 is unused (U1000)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: test-flakiness
🔇 Additional comments (1)
internal/stage_p3.go (1)

63-66: Consider using appendFile synchronously after file.Sync

If you incorporate the earlier file.Sync() change, the appended line will be visible to tail -f immediately, reducing flakiness in fast CI environments.

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: 5

♻️ Duplicate comments (2)
internal/stage_p2.go (1)

17-22: 💡 Verification agent

🧩 Analysis chain

Verify the command type handling in SetUpCustomCommands

The code correctly sets up a custom wc command, but based on past review comments, the SetUpCustomCommands function may not properly handle the "wc" command type yet.


🏁 Script executed:

#!/bin/bash
# Check if SetUpCustomCommands handles the "wc" command type
grep -n "case \"wc\":" internal/utils_build.go

Length of output: 46


Let’s locate the SetUpCustomCommands implementation to inspect its command‐type handling:


🏁 Script executed:

#!/bin/bash
# Find where SetUpCustomCommands is defined
grep -R -n "func SetUpCustomCommands" .

Length of output: 256


To verify if "wc" is handled, let’s dump the body of SetUpCustomCommands for inspection:


🏁 Script executed:

#!/bin/bash
# Print lines 27–200 of internal/utils_build.go to view the entire SetUpCustomCommands implementation
sed -n '27,200p' internal/utils_build.go

Length of output: 1501


Add missing “wc” handling in SetUpCustomCommands

The loop in internal/utils_build.go only handles "ls", "cat", and "signature_printer", so your custom wc command is never created. Please add a case "wc": block analogous to the others:

• File: internal/utils_build.go
• Function: SetUpCustomCommands (lines ~33–47)

Suggested diff:

     for _, commandDetail := range commands {
       switch commandDetail.CommandType {
         case "ls":
           customLsPath := path.Join(executableDir, commandDetail.CommandName)
           err = custom_executable.CreateLsExecutable(customLsPath)
           if err != nil {
             return "", err
           }
         case "cat":
           customCatPath := path.Join(executableDir, commandDetail.CommandName)
           err = custom_executable.CreateCatExecutable(customCatPath)
           if err != nil {
             return "", err
           }
         case "signature_printer":
           // …existing code…
+        case "wc":
+          customWcPath := path.Join(executableDir, commandDetail.CommandName)
+          err = custom_executable.CreateWcExecutable(customWcPath)
+          if err != nil {
+            return "", err
+          }
       }
     }

Ensure you have a CreateWcExecutable helper in custom_executable and add tests to cover the new command type.

internal/stage_p1.go (1)

49-50: ⚠️ Potential issue

Update wc output format to match implementation

The format string for wc output doesn't match the implementation. According to past review comments, all fields should use %8d.

-expectedOutput := fmt.Sprintf("%7d%8d%8d", lines, words, bytes)
+expectedOutput := fmt.Sprintf("%8d%8d%8d", lines, words, bytes)
🧹 Nitpick comments (6)
internal/custom_executable/grep/grep_test.go (4)

48-94: Consider separating build logic from test execution

The current implementation builds the custom grep executable inside the test runner function. This could potentially slow down tests if runGrep is called multiple times, as it would attempt to build the executable each time.

Consider moving the build step to a separate function that's called once in a TestMain or using a sync.Once mechanism to ensure it only builds once:

var buildOnce sync.Once

func ensureGrepBuilt(t *testing.T) {
    if *useSystemGrep {
        return
    }
    
    buildOnce.Do(func() {
        buildCmd := exec.Command("go", "build", "-o", "grep", ".")
        buildOutput, err := buildCmd.CombinedOutput()
        if err != nil {
            t.Fatalf("Failed to build grep executable for testing: %v\nOutput:\n%s", err, string(buildOutput))
        }
    })
    
    // Keep cleanup in the test that requested the build
    t.Cleanup(func() {
        os.Remove("./grep")
    })
}
🧰 Tools
🪛 GitHub Check: lint

[failure] 87-87:
empty branch (SA9003)


149-157: Consider additional test cases for grep functionality

The current test suite covers basic grep functionality, but could be expanded to test other common grep use cases.

Consider adding tests for:

  • Case-insensitive matching with -i flag
  • Regular expression patterns (if supported)
  • Multiple occurrences in the same line
  • Special characters in patterns (e.g., ., *, [])

163-168: Improve test skipping with t.Skip()

The current approach to skipping the "Usage error" test when using system grep simply returns from the test function, which doesn't provide clear indication in test output that a test was skipped.

if *useSystemGrep && tt.name == "Usage error - no pattern" {
-   return // Skip this test for System grep
+   t.Skip("Skipping 'Usage error' test when using system grep")
}

203-209: Plan for file-based grep testing implementation

The TODOs for file-based testing are a good reminder for future work, but could be more specific.

The placeholders for file-based testing indicate an important future enhancement. Consider implementing these tests soon for comprehensive coverage of the grep functionality. Would you like me to help draft a more detailed plan or implementation for the file-based testing functionality?

internal/test_helpers/course_definition.yml (2)

939-944: Add newline at end of file

YAML files should end with a newline character as per POSIX standards. This is flagged by the YAMLlint static analysis tool.

  - slug: "xk3"
+
🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 944-944: no new line character at the end of file

(new-line-at-end-of-file)


939-944: Document the purpose of new stage slugs

Consider adding comments to explain what each new stage slug represents. This would improve maintainability and make it easier for other developers to understand the purpose of each stage.

  - slug: "br6"  # Pipeline test 1: cat|wc and tail|head
  - slug: "p2"   # Pipeline test 2: echo|wc and ls|type
  - slug: "p3"   # Pipeline test 3: [description]
  - slug: "ny9"  # [description]
  - slug: "p5"   # [description]
  - slug: "xk3"  # [description]
🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 944-944: no new line character at the end of file

(new-line-at-end-of-file)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 87b4c78 and 8dd88a3.

📒 Files selected for processing (8)
  • Makefile (6 hunks)
  • internal/custom_executable/grep/grep.go (1 hunks)
  • internal/custom_executable/grep/grep_test.go (1 hunks)
  • internal/stage_p1.go (1 hunks)
  • internal/stage_p2.go (1 hunks)
  • internal/stage_p3.go (1 hunks)
  • internal/test_helpers/course_definition.yml (6 hunks)
  • internal/tester_definition.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • internal/custom_executable/grep/grep.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal/stage_p3.go
  • internal/tester_definition.go
  • Makefile
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/stage_p2.go (5)
internal/shell_executable/shell_executable.go (1)
  • NewShellExecutable (42-55)
internal/utils_build.go (2)
  • SetUpCustomCommands (27-68)
  • CommandDetails (16-25)
internal/utils.go (1)
  • CUSTOM_WC_COMMAND (18-18)
internal/logged_shell_asserter/logged_shell_asserter.go (1)
  • NewLoggedShellAsserter (25-37)
internal/test_cases/command_response_test_case.go (1)
  • CommandResponseTestCase (18-30)
🪛 GitHub Check: lint
internal/custom_executable/grep/grep_test.go

[failure] 87-87:
empty branch (SA9003)

🪛 YAMLlint (1.35.1)
internal/test_helpers/course_definition.yml

[error] 944-944: no new line character at the end of file

(new-line-at-end-of-file)

🔇 Additional comments (5)
internal/custom_executable/grep/grep_test.go (2)

22-46: Ensure consistent handling for all supported platforms

The getGrepExecutable function handles darwin and linux for arm64 and amd64, but doesn't specifically handle other potential platforms.

The implementation correctly handles the common platforms and fails appropriately for unsupported ones. The code is well-structured and clear in its intent.


106-201: Thorough stdin testing implementation

The tests for stdin functionality cover the main use cases well and effectively verify both successful and error scenarios.

The test structure with table-driven tests is well-organized and follows Go testing best practices. The comprehensive verification of exit codes, stdout, and stderr ensures proper functionality validation.

internal/stage_p2.go (2)

39-47: LGTM! Good use of the CommandResponseTestCase structure

The test case is well structured and properly asserts the expected output.


51-62: Comprehensive pipeline test with builtin command

This is a good test case that checks both pipeline functionality and the ability to identify shell builtins inside a pipeline.

internal/stage_p1.go (1)

95-123: Well-implemented interactive file modification test

This is a great test case that verifies dynamic content updates using tail and appending to files during test execution. The assertions are properly set up to verify the content appears without prompt.

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: 1

🧹 Nitpick comments (5)
internal/test_helpers/course_definition.yml (1)

941-941: Add a newline at end of file

The YAML file is missing a newline at the end, which is a POSIX standard requirement and could cause parsing issues.

-  - slug: "xk3"
+  - slug: "xk3"
+
🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 941-941: no new line character at the end of file

(new-line-at-end-of-file)

internal/stages_test.go (1)

115-121: Ensure ASH pipeline fixture alignment with BASH counterpart

The new ASH pipeline test case uses the same stage slugs as the BASH test case. Make sure both fixtures have consistent expectations and account for any shell-specific differences.

internal/test_helpers/fixtures/ash/pipelines/pass (3)

1-2: Consider making debug mode conditional

Debug mode is permanently enabled. Consider making it conditional based on an environment variable to avoid verbose output in normal runs.

-Debug = true
+Debug = ${DEBUG:-false}
🧰 Tools
🪛 GitHub Actions: Test

[error] 1-1: Test failure in 'TestStages/pipelines_pass_bash' causing 'TestStages' to fail. Differences detected in output compared to expected results.


11-11: Remove duplicated file setup

The same setup command is executed twice in a row, which is unnecessary.

 [33m[stage-3] [setup] [0m[94mecho -n "orange blueberry\nstrawberry pineapple\nbanana mango" > "/tmp/baz/file-3"[0m
-[33m[stage-3] [setup] [0m[94mecho -n "orange blueberry\nstrawberry pineapple\nbanana mango" > "/tmp/baz/file-3"[0m

52-53: Add newline at end of file

This file is missing a newline at the end.

 [33m[stage-1] [0m[92mTest passed.[0m
+
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 8dd88a3 and ae742f0.

📒 Files selected for processing (6)
  • .gitignore (1 hunks)
  • internal/stage_p2.go (1 hunks)
  • internal/stages_test.go (2 hunks)
  • internal/test_helpers/course_definition.yml (6 hunks)
  • internal/test_helpers/fixtures/ash/pipelines/pass (1 hunks)
  • internal/test_helpers/fixtures/bash/pipelines/pass (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • internal/test_helpers/fixtures/bash/pipelines/pass
🚧 Files skipped from review as they are similar to previous changes (2)
  • .gitignore
  • internal/stage_p2.go
🧰 Additional context used
🪛 GitHub Actions: Test
internal/stages_test.go

[error] 1-1: Test failure in 'TestStages/pipelines_pass_bash' causing 'TestStages' to fail. Differences detected in output compared to expected results.

internal/test_helpers/fixtures/ash/pipelines/pass

[error] 1-1: Test failure in 'TestStages/pipelines_pass_bash' causing 'TestStages' to fail. Differences detected in output compared to expected results.

internal/test_helpers/course_definition.yml

[error] 1-1: Test failure in 'TestStages/pipelines_pass_bash' causing 'TestStages' to fail. Differences detected in output compared to expected results.

🪛 YAMLlint (1.35.1)
internal/test_helpers/course_definition.yml

[error] 941-941: no new line character at the end of file

(new-line-at-end-of-file)

🔇 Additional comments (1)
internal/test_helpers/fixtures/ash/pipelines/pass (1)

12-20:

❌ Incorrect review comment

Missing line append test verification

The test shows output for appended lines 4 and 5, but there's no setup command showing when these lines are appended. The verification is present, but the operation that adds the lines is missing.


🏁 Script executed:

#!/bin/bash
# Check if there's a corresponding test_helpers file that might have the missing append operations

# Look for the test implementation that corresponds to stage br6
echo "Looking for Stage br6 implementation..."
rg -l "br6" --type go internal/
rg -A 10 "appendFile" --type go internal/

Length of output: 1838


No missing append operations—test harness appends lines in stage_p1.go

The appendFile calls in internal/stage_p1.go already simulate adding lines 4 and 5, so the original concern about missing setup is unfounded.

• internal/stage_p1.go:

  • appendFile(filePath, "This is line 4.\n")
  • appendFile(filePath, "This is line 5.\n")

Please ignore the original comment.

Likely an incorrect or invalid review comment.

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)
internal/utils_build.go (1)

52-96: Consider refactoring the repeated pattern in the switch cases.

The implementation is correct and follows a consistent pattern across all command types. However, there's significant code duplication that could be refactored to improve maintainability.

 func SetUpCustomCommands(stageHarness *test_case_harness.TestCaseHarness, shell *shell_executable.ShellExecutable, commands []CommandDetails, useShorterDirectory bool) (string, error) {
     // ... existing code ...
     
     for _, commandDetail := range commands {
-        switch commandDetail.CommandType {
-        case "cat":
-            customCatPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateCatExecutable(customCatPath)
-            if err != nil {
-                return "", err
-            }
-        case "grep":
-            customGrepPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateGrepExecutable(customGrepPath)
-            if err != nil {
-                return "", err
-            }
-        case "head":
-            customHeadPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateHeadExecutable(customHeadPath)
-            if err != nil {
-                return "", err
-            }
-        case "ls":
-            customLsPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateLsExecutable(customLsPath)
-            if err != nil {
-                return "", err
-            }
-        case "tail":
-            customTailPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateTailExecutable(customTailPath)
-            if err != nil {
-                return "", err
-            }
-        case "wc":
-            customWcPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateWcExecutable(customWcPath)
-            if err != nil {
-                return "", err
-            }
-        case "yes":
-            customYesPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateYesExecutable(customYesPath)
-            if err != nil {
-                return "", err
-            }
+        executablePath := path.Join(executableDir, commandDetail.CommandName)
+        switch commandDetail.CommandType {
+        case "cat":
+            err = custom_executable.CreateCatExecutable(executablePath)
+        case "grep":
+            err = custom_executable.CreateGrepExecutable(executablePath)
+        case "head":
+            err = custom_executable.CreateHeadExecutable(executablePath)
+        case "ls":
+            err = custom_executable.CreateLsExecutable(executablePath)
+        case "tail":
+            err = custom_executable.CreateTailExecutable(executablePath)
+        case "wc":
+            err = custom_executable.CreateWcExecutable(executablePath)
+        case "yes":
+            err = custom_executable.CreateYesExecutable(executablePath)
         case "signature_printer":
-            customSignaturePrinterPath := path.Join(executableDir, commandDetail.CommandName)
-            err = custom_executable.CreateSignaturePrinterExecutable(commandDetail.CommandMetadata, customSignaturePrinterPath)
-            if err != nil {
-                return "", err
-            }
+            err = custom_executable.CreateSignaturePrinterExecutable(commandDetail.CommandMetadata, executablePath)
         default:
             panic(fmt.Sprintf("CodeCrafters Internal Error: unknown command type %s", commandDetail.CommandType))
         }
+        if err != nil {
+            return "", err
+        }
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between fed0f89 and dc6b11d.

📒 Files selected for processing (2)
  • internal/custom_executable/build/grep.go (1 hunks)
  • internal/utils_build.go (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • internal/custom_executable/build/grep.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/utils_build.go (7)
internal/custom_executable/build/cat.go (1)
  • CreateCatExecutable (5-12)
internal/custom_executable/build/grep.go (1)
  • CreateGrepExecutable (5-11)
internal/custom_executable/build/head.go (1)
  • CreateHeadExecutable (5-11)
internal/custom_executable/build/ls.go (1)
  • CreateLsExecutable (5-11)
internal/custom_executable/build/tail.go (1)
  • CreateTailExecutable (5-11)
internal/custom_executable/build/wc.go (1)
  • CreateWcExecutable (5-11)
internal/custom_executable/build/yes.go (1)
  • CreateYesExecutable (5-11)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: test-flakiness
🔇 Additional comments (4)
internal/utils_build.go (4)

4-4: Good addition of the "fmt" import.

This import is appropriately added to support the formatted panic message in the new default case.


24-24: Properly updated metadata comment.

The comment has been correctly updated to reflect all the command types that require no metadata.


46-51: Consolidation of the "cat" case is good.

The implementation follows the established pattern and makes consistent use of the error handling pattern.


94-95: Good defensive programming with panic for unknown command types.

Using panic for internal development errors is appropriate here, as this is a development-time check that should never happen in production. The error message clearly indicates that this is an internal error.

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: 1

♻️ Duplicate comments (1)
internal/custom_executable/grep/grep_test.go (1)

66-72: ⚠️ Potential issue

Missing handling for non-exit errors

The runGrep function correctly extracts the exit code for ExitError types but doesn't handle other types of errors that might occur during command execution. This was previously flagged in static analysis and addressed in an earlier commit, but appears to be missing in this version.

if err != nil {
  var exitError *exec.ExitError
  if errors.As(err, &exitError) {
    exitCode = exitError.ExitCode()
  } else {
+   // If it's not an ExitError, it might be a different execution problem (e.g., command not found)
+   t.Logf("Non-exit error occurred: %v", err)
  }
}
🧹 Nitpick comments (5)
internal/custom_executable/grep/grep_test.go (5)

48-75: Consider adding error detail for debugging

The runGrep function returns the error, but it would be helpful to provide more context about what command was executed when an error occurs. This helps with debugging test failures.

func runGrep(t *testing.T, stdinContent string, args ...string) (string, string, int, error) {
	executable := getGrepExecutable(t)

	t.Helper()
	prettyPrintCommand(args)
	cmd := exec.Command(executable, args...)

	if stdinContent != "" {
		cmd.Stdin = strings.NewReader(stdinContent)
	}

	var stdout, stderr bytes.Buffer
	cmd.Stdout = &stdout
	cmd.Stderr = &stderr

	err := cmd.Run()

	exitCode := 0
	if err != nil {
		var exitError *exec.ExitError
		if errors.As(err, &exitError) {
			exitCode = exitError.ExitCode()
		} else {
+			// Wrap non-exit errors with command context
+			err = fmt.Errorf("executing grep %s: %w", strings.Join(args, " "), err)
+			t.Logf("Non-exit error occurred: %v", err)
		}
	}

	return stdout.String(), stderr.String(), exitCode, err
}

77-85: Implement or remove the placeholder comment

There's a comment about potentially shortening paths or quoting arguments, but no implementation. Either implement this functionality or remove the comment to avoid confusion.

func prettyPrintCommand(args []string) {
	// Basic pretty printing, similar to cat/wc tests
	displayArgs := make([]string, len(args))
	copy(displayArgs, args)
-	// Potentially shorten paths or quote arguments if needed here

	out := fmt.Sprintf("=== RUN:  > grep %s", strings.Join(displayArgs, " "))
	fmt.Println(out)
}

121-129: Clarify expectation for empty pattern behavior

The comment mentions that "Real grep might error or behave differently with empty pattern" but doesn't specify what the expected behavior is for your custom implementation. This would be helpful for understanding the test case.

{
	name:         "Match on empty string pattern",
	pattern:      "", // Our simple strings.Contains matches everything
	input:        "line1\nline2",
	expectedOut:  "line1\nline2\n",
	expectedErr:  "",
	expectedExit: 0,
-	// Note: Real grep might error or behave differently with empty pattern
+	// Note: BSD grep treats empty pattern as match nothing, GNU grep treats it as error.
+	// Our implementation matches every line with an empty pattern.
},

144-149: Improve handling of "Usage error - no pattern" test

The handling of the "Usage error - no pattern" test case is a bit confusing. You skip it for system grep but keep the pattern empty for custom grep. Consider refactoring for clarity.

if *useSystemGrep && tt.name == "Usage error - no pattern" {
+	t.Skip("Skipping usage error test for system grep - behavior differs")
-	return // Skip this test for System grep
}
-if tt.name != "Usage error - no pattern" {
+
+// Only add pattern to args if this isn't the usage error test
+if tt.pattern != "" || tt.name != "Usage error - no pattern" {
	args = append(args, tt.pattern)
-} // else args remains empty
}

170-179: Simplify error handling logic

The error handling logic is a bit complex. You can simplify it by focusing on the specific error conditions you're trying to check.

-// Handle expected usage error specifically
-if tt.expectedExit == 2 && runErr == nil {
-	t.Errorf("Expected a non-nil error for usage error case, but got nil")
-} else if tt.expectedExit != 2 && runErr != nil {
-	// If we didn't expect an error exit, but got one, check if it was *exec.ExitError
-	if _, ok := runErr.(*exec.ExitError); !ok {
-		// It was some other error during execution
-		t.Errorf("Command execution failed unexpectedly: %v", runErr)
-	}
-	// If it *was* an ExitError, we already checked the exit code above.
+// Check error expectations
+switch {
+case tt.expectedExit != 0 && runErr == nil:
+	t.Errorf("Expected an error for exit code %d, but got nil", tt.expectedExit)
+case tt.expectedExit == 0 && runErr != nil:
+	t.Errorf("Expected success (exit 0), but got error: %v", runErr)
+case runErr != nil && !errors.As(runErr, &exec.ExitError{}):
+	// Got an error that's not an ExitError
+	t.Errorf("Command execution failed with unexpected error type: %T: %v", runErr, runErr)
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between dd8567f and 51adce2.

📒 Files selected for processing (3)
  • Makefile (6 hunks)
  • internal/custom_executable/grep/grep_test.go (1 hunks)
  • internal/utils_build.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • Makefile
  • internal/utils_build.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: test-flakiness
🔇 Additional comments (3)
internal/custom_executable/grep/grep_test.go (3)

16-20: Good use of flags and documentation

The -system flag is well-documented with clear usage instructions and explanation about BSD vs GNU grep differences. This makes the test easier to run and understand.


87-138: Well-structured test cases with good coverage

The test cases cover a variety of scenarios including matches, no matches, empty input, empty pattern, and usage errors. Each test case has clear expected outputs and exit codes.


152-158: Good error message with comprehensive debug info

The error message includes stdout, stderr, and the original error when the exit code doesn't match. This is excellent practice that makes debugging test failures much easier.

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 (2)
internal/custom_executable/tail/tail_test.go (2)

589-595: Consider handling the io.EOF case with errors.Is() as well

While this change correctly uses errors.Is() for io.ErrClosedPipe, consider applying the same approach to io.EOF for consistency and future-proofing.

-if err == io.EOF || errors.Is(err, io.ErrClosedPipe) {
+if errors.Is(err, io.EOF) || errors.Is(err, io.ErrClosedPipe) {

609-615: Consider handling the io.EOF case with errors.Is() as well

Similar to the stdout handler, consider using errors.Is() for the io.EOF check here as well for consistency.

-if err == io.EOF || errors.Is(err, io.ErrClosedPipe) {
+if errors.Is(err, io.EOF) || errors.Is(err, io.ErrClosedPipe) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 51adce2 and 076853e.

📒 Files selected for processing (4)
  • internal/custom_executable/grep/grep_test.go (1 hunks)
  • internal/custom_executable/head/head.go (3 hunks)
  • internal/custom_executable/tail/tail_test.go (2 hunks)
  • internal/test_cases/command_multiple_completions_test_case.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/custom_executable/head/head.go
  • internal/custom_executable/grep/grep_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test-flakiness
  • GitHub Check: test-all-shells
🔇 Additional comments (3)
internal/test_cases/command_multiple_completions_test_case.go (1)

70-70: Clean boolean expression - good improvement!

The simplification of the boolean expression makes the code more readable while maintaining the same logic. This is a good practice for code clarity.

internal/custom_executable/tail/tail_test.go (2)

589-589: Improved error handling by using errors.Is for wrapped errors

This change correctly uses errors.Is() instead of direct equality checking for io.ErrClosedPipe, which is the recommended approach for handling potentially wrapped errors in Go.


609-609: Consistent error handling improvement

Good symmetrical implementation of errors.Is() in the stderr handling code, matching the stdout implementation approach.

@ryan-gang ryan-gang requested a review from rohitpaulk April 29, 2025 09:03
@ryan-gang ryan-gang merged commit ccb05ad into main Apr 30, 2025
5 of 6 checks passed
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: 2

🧹 Nitpick comments (1)
internal/test_helpers/course_definition.yml (1)

1283-1283: Add newline at end of file
YAML files should end with a newline to satisfy POSIX standards and avoid parsing issues.

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 1283-1283: no new line character at the end of file

(new-line-at-end-of-file)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 4baeada and 4894ec3.

📒 Files selected for processing (1)
  • internal/test_helpers/course_definition.yml (11 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
internal/test_helpers/course_definition.yml

[error] 1283-1283: no new line character at the end of file

(new-line-at-end-of-file)

🔇 Additional comments (19)
internal/test_helpers/course_definition.yml (19)

308-311: Approve: expanded PATH in The type builtin: executable files stage
Including /usr/local/bin in the test PATH ensures executables in that directory are discoverable. This aligns with real-world environments.


329-332: Approve: clarifying note for builtins vs executables
Adding the note that commands like echo should still be treated as builtins when they exist in $PATH improves test clarity and reduces ambiguity.


362-364: Approve: clarity on executable behavior in Run a program stage
The new sentence explaining that the custom executable prints its own signature helps avoid misinterpretation of the test’s intent.


366-369: Approve: note about not implementing output manually
Stating that the sample output comes from the executable itself rather than your shell implementation is a helpful clarification.


573-577: Approve: added cat test in single-quotes stage
Including file names with spaces under single quotes ensures users handle quoted filenames correctly. This test is essential for real-world quoting support.


581-585: Approve: reminder that cat is provided by the system
The added bullet clarifying that cat is an external executable and not something to implement is on point.


618-622: Approve: added cat test in double-quotes stage
Testing cat with filenames containing embedded quotes validates proper parsing of double-quoted paths.


658-662: Approve: added cat test in backslash-escaping stage
This ensures that backslashes outside of quotes are handled and filenames with backslashes are parsed correctly.


952-986: Request verification: ensure implementation of the qp2 (Builtin completion) stage
You’ve added the qp2 stage under completions. Confirm that a corresponding testP1 (or similar) function exists in the Go code, that it’s wired into the tester definition, and that fixtures under fixtures/(bash|ash)/completions/ cover it.


989-999: Request verification: ensure implementation of the gm9 (Completion with arguments) stage
The gm9 stage was appended. Verify that a testP2 function and YAML binding exist, and that fixtures validate argument-aware completion.


1025-1031: Request verification: ensure implementation of the qm8 (Missing completions) stage
Adding the qm8 stage requires a matching Go test (testP3) and updated tester definitions. Please confirm the stage is exercised in your pipeline.


1053-1060: Request verification: ensure implementation of the gy5 (Executable completion) stage
The new stage under completions must be backed by code in internal/stage_* and fixtures for executable completion. Double-check wiring in tester_definition.go.


1085-1092: Request verification: ensure implementation of the wh6 (Multiple completions) stage
Confirm that both the two-press bell and listing behaviors for multiple matches are implemented, and that the stage is included in end-to-end tests.


1125-1132: Request verification: ensure implementation of the wt6 (Partial completions) stage
This complex stage needs a dedicated test harness and must appear in the fixtures directory. Please verify the Go stage and fixtures cover all prefix-cycling scenarios.


1170-1180: Request verification: ensure implementation of the br6 (Dual-command pipeline) stage
You’ve registered br6 under pipelines. Make sure testP1 (or equivalent) is implemented, that internal/custom_executable code supports piping, and fixtures under fixtures/.../pipelines/ exist.


1212-1220: Request verification: ensure implementation of the ny9 (Pipelines with built-ins) stage
The built-in pipeline tests may require special handling in your shell. Verify that the code paths for built-ins within pipelines are fully exercised by fixtures and Go tests.


1248-1254: Request verification: ensure implementation of the xk3 (Multi-command pipelines) stage
Multi-stage pipelines introduce complexity around pipe chaining and process cleanup. Confirm that both code and tests address three-plus command sequences as defined here.


18-23:

❓ Verification inconclusive

Verify support for the new "gleam" language slug
You’ve added - slug: "gleam" to the list of supported languages. Ensure that the rest of the tooling (build scripts, template rendering, syntax highlighting, etc.) recognizes this new slug and that any lookup tables have been updated accordingly.

Run a quick search to confirm no references to "gleam" were missed:


🏁 Script executed:

#!/bin/bash
# Verify that all code paths include support for "gleam"
rg -n 'gleam' 

Length of output: 78


gleam support requires additional updates across tooling
A search for "gleam" only returned the new slug in internal/test_helpers/course_definition.yml. Please ensure that every part of our toolchain recognizes this language:

  • Update any language‐lookup tables or enums used in build scripts
  • Add “gleam” to template rendering logic (e.g., course pages, code sandbox configs)
  • Confirm syntax highlighting configs (Prism, highlight.js, etc.) include gleam
  • Verify CI jobs, linters or formatters that enumerate supported languages
  • Run rg -n 'gleam' across your configs and code to catch any remaining spots

74-87:

❓ Verification inconclusive

Confirm integration of the new completions and pipelines extensions
You’ve introduced two new challenge extensions under extensions. Please verify that these slugs are registered in your internal course loader and that any UI or validation logic has been updated to handle them.

For example, check that the loader references these slugs:


🏁 Script executed:

rg -n 'completions' -n 'pipelines'

Length of output: 82


🏁 Script executed:

#!/bin/bash
# Search for usages of the new extension slugs in the codebase
rg -n -e 'completions' -e 'pipelines' .

Length of output: 4602


#!/bin/bash
# List all Go source files under the internal directory to locate course loader implementation
fd --type f --ext go internal

#!/bin/bash
# Search for YAML unmarshalling in Go code
rg -n 'Unmarshal' -t go

# Look for code reading or referencing course_definition.yml
rg -n 'course_definition.yml' -t go

# Search for any loader functions dealing with extensions or slugs
rg -n 'Load' -e 'extension' -t go

🏁 Script executed:

#!/bin/bash
# Locate references to the course_definition.yml loader
rg -n 'course_definition.yml' -t go

# Find YAML unmarshalling calls
rg -n 'yaml.Unmarshal' -t go

# Find file reads for loading config
rg -n 'ReadFile' -t go

Length of output: 516


🏁 Script executed:

#!/bin/bash
# Display the context around the YAML validation in tester_definition_test.go
sed -n '1,200p' internal/tester_definition_test.go

Length of output: 320


#!/bin/bash
# Search for the declaration of testerDefinition in the internal package
rg -n 'testerDefinition' -t go .

🏁 Script executed:

#!/bin/bash
# Locate declaration and usage of testerDefinition in Go code
rg -n 'testerDefinition' -t go .

Length of output: 449


#!/bin/bash
# Display the initialization of testerDefinition to verify registered extensions
sed -n '1,200p' internal/tester_definition.go

Verify loader registration of new completions and pipelines extensions
Please confirm that both slugs are wired up in your course loader and validation logic (e.g. in internal/tester_definition.go). For example, run:

rg -n 'completions' internal/tester_definition.go
rg -n 'pipelines' internal/tester_definition.go

Then execute the YAML validation test (go test -run TestStagesMatchYAML) to ensure no missing entries for these extensions.

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