The error messages should be more descriptive now #120
Conversation
WalkthroughThis change introduces enhanced error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant ShellScript
participant PythonShell
Tester->>ShellScript: Run your_shell.sh
ShellScript->>PythonShell: Execute main.py
PythonShell->>Tester: Display prompt "$ "
Tester->>PythonShell: Send command (e.g., invalid_apple_command)
PythonShell->>Tester: Print "invalid_apple_command: command not found"
PythonShell->>Tester: Display prompt "$ "
Tester->>PythonShell: Send "exit 0"
PythonShell->>Tester: Print "exit: command not found"
PythonShell->>Tester: Display prompt "$ "
Tester->>Tester: Detects program did not exit, logs error
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
lemme try fixing, pls hold off on review |
|
ok now (I rebuilt some executables but didn't have all tools on my system) |
|
@Arpan-206 the error message is still confusing. Why are we expecting a prompt to appear after running exit? And why are we printing program output in red? A user's program output should always just appear under |
Arpan-206
force-pushed
the
arpan/cc-1667-fix-incorrect-shell-tester-message-for-stage-pn5
branch
from
0e6f4ac to
aec290e
Compare
There was a problem hiding this comment.
@Arpan-206 let's make it a point to update the description with newer screenshots - can't make sense of what the new error message is now. Secondly, let's add tests for this as mentioned in Linear - check stages_test.go for examples
New SS, adding tests!
|
Arpan-206
force-pushed
the
arpan/cc-1667-fix-incorrect-shell-tester-message-for-stage-pn5
branch
from
92d6455 to
2b2804c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/test_helpers/scenarios/exit_error/codecrafters.yml (1)
1-5: Fix YAML formatting issuesThere are two minor formatting issues in this file:
- There are trailing spaces at the end of line 5
- There's no newline at the end of the file
# Set this to true if you want debug logs. # # These can be VERY verbose, so we suggest turning them off # unless you really need them. -debug: true +debug: true +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
[error] 5-5: trailing spaces
(trailing-spaces)
internal/test_helpers/scenarios/exit_error/your_shell.sh (1)
8-8: Fix potential word splitting issue in command substitutionThe
$(dirname "$0")command substitution should be quoted to prevent word splitting if the path contains spaces.-exec python3 $(dirname "$0")/main.py "$@" +exec python3 "$(dirname "$0")"/main.py "$@" +This change also adds a trailing newline at the end of the file.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 8-8: Quote this to prevent word splitting.
(SC2046)
internal/test_helpers/scenarios/exit_error/main.py (2)
12-12: Remove unused variable assignmentThe
argsvariable is assigned but never used in the code.command = input().strip() parts = command.split(" ") cmd = parts[0] - args = parts[1:] + # args = parts[1:] # Uncomment when needed🧰 Tools
🪛 Ruff (0.8.2)
12-12: Local variable
argsis assigned to but never usedRemove assignment to unused variable
args(F841)
21-22: Add newline at end of fileThe file is missing a newline at the end.
if __name__ == "__main__": - main() + main() +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
internal/stage4.go(2 hunks)internal/stages_test.go(1 hunks)internal/test_helpers/fixtures/bash/base/pass(1 hunks)internal/test_helpers/fixtures/bash/navigation/pass(1 hunks)internal/test_helpers/fixtures/exit_error(1 hunks)internal/test_helpers/scenarios/exit_error/codecrafters.yml(1 hunks)internal/test_helpers/scenarios/exit_error/main.py(1 hunks)internal/test_helpers/scenarios/exit_error/your_shell.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- internal/test_helpers/fixtures/bash/navigation/pass
- internal/test_helpers/fixtures/bash/base/pass
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/stage4.go
🧰 Additional context used
🪛 Ruff (0.8.2)
internal/test_helpers/scenarios/exit_error/main.py
12-12: Local variable args is assigned to but never used
Remove assignment to unused variable args
(F841)
🪛 YAMLlint (1.35.1)
internal/test_helpers/scenarios/exit_error/codecrafters.yml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
[error] 5-5: trailing spaces
(trailing-spaces)
🪛 Shellcheck (0.10.0)
internal/test_helpers/scenarios/exit_error/your_shell.sh
[warning] 8-8: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (6)
internal/stages_test.go (1)
36-42: Well-structured test case for the new exit error scenarioThe new test case is properly structured and follows the same pattern as other test cases. It correctly references the new scenario path and fixture output.
internal/test_helpers/scenarios/exit_error/main.py (1)
14-15:✅ Verification successful
Verify intentional misspelling of "exitt" command
The code only recognizes "exitt" (with two 't's) instead of the standard "exit" command. This appears to be intentional for testing purposes, but please confirm this is the desired behavior.
🏁 Script executed:
#!/bin/bash # Check if this behavior is consistent with other test scenarios in the codebase grep -r "exit" --include="*.py" internal/test_helpers/scenarios/Length of output: 217
Confirmed intentional misspelling in exit_error scenario
A recursive search for “exit” across all test scenarios only finds theexittcheck and correspondingsys.exit(0)ininternal/test_helpers/scenarios/exit_error/main.py. This appears deliberate to differentiate it from the normal “exit” command. No changes are needed.internal/test_helpers/fixtures/exit_error (4)
1-2: LGTM: Debug mode enables better traceability.Debug flag is properly set to true, which will help in troubleshooting this test scenario by providing more detailed logs.
3-7: LGTM: Test sequence correctly validates command not found behavior.The test correctly verifies that an invalid command produces the expected "command not found" error message, which is a fundamental shell behavior.
8-10: Effectively captures the exit command failure.This section effectively demonstrates the specific error case - where a shell incorrectly treats
exitas an external command rather than a built-in shell directive. This is a crucial test case as implementing built-ins likeexitis a fundamental requirement for a functioning shell.
11-12: Clear error messaging aligns with PR objectives.The error message is now more descriptive and clearly communicates what went wrong (program should have exited but is still running). This directly addresses the PR objective of improving error message clarity.
New error message. |
internal/stage4.go
Outdated
| @@ -47,6 +48,9 @@ func testExit(stageHarness *test_case_harness.TestCaseHarness) error { | |||
| if !errors.Is(readErr, shell_executable.ErrProgramExited) { | |||
| if readErr == nil { | |||
| return fmt.Errorf("Expected program to exit with 0 exit code, program is still running.") | |||
| } else if errors.Is(readErr, condition_reader.ErrConditionNotMet) { | |||
| asserter.LogRemainingOutput() | |||
There was a problem hiding this comment.
@Arpan-206 thought we talked about this - we should be logging this no matter what, not only when a specific error type is found
There was a problem hiding this comment.
Oh I thought I changed it back, lemme do that!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/logged_shell_asserter/logged_shell_asserter.go (1)
118-139: Consider adding debug logging for boundary errorsAll these fixes address important off-by-one errors in row boundary calculations. Since these types of boundary errors can be subtle, consider adding some debug logging to help identify similar issues in the future.
func (a *LoggedShellAsserter) logRows(startRowIndex int, endRowIndex int) { + // Debug logging for boundary calculations (consider using a debug log level) + // a.Shell.GetLogger().Debugf("Logging rows from %d to %d (total rows: %d)", + // startRowIndex, endRowIndex, len(a.Shell.GetScreenState())) for i := startRowIndex; i <= endRowIndex; i++ { rawRow := a.Shell.GetScreenState()[i] cleanedRow := virtual_terminal.BuildCleanedRow(rawRow) if len(cleanedRow) > 0 { a.Shell.LogOutput([]byte(cleanedRow)) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
internal/logged_shell_asserter/logged_shell_asserter.go(1 hunks)internal/stage4.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/stage4.go
🔇 Additional comments (3)
internal/logged_shell_asserter/logged_shell_asserter.go (3)
118-121: Fixed off-by-one error in error row loggingThe changes correctly adjust the row indices in the
logAssertionErrormethod. This ensures that all rows between the last logged row and the error row, as well as all rows after the error, are properly displayed in the logs.
124-129: Improved remaining output logging and trackingThese changes fix two important issues:
- Setting the proper end index for logging (using length-1 instead of length)
- Updating the lastLoggedRowIndex to correctly reflect the last row that was logged
This ensures that when logging remaining output, we accurately track which rows have already been displayed and avoid missing or duplicating rows in the logs.
131-132: Fixed loop condition to include the end rowChanged the loop condition from
i < endRowIndextoi <= endRowIndexto ensure the row at the end index is also included in the logs. This correctly implements the expected behavior of thelogRowsmethod.
Arpan-206
deleted the
arpan/cc-1667-fix-incorrect-shell-tester-message-for-stage-pn5
branch
Before:
After:
After (outdated):
After (outdated, so disregard):
Summary by CodeRabbit
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
Chores