Background Jobs (Bootstrap + Stage 1-2)#230
Conversation
…estCase and JobsBuiltinTestCase
…ition; record fixtures
This comment has been minimized.
This comment has been minimized.
…ponseTestCase The second AssertWithPrompt() call on line 67 was redundant because: - No new assertions were added after the first call on line 43 - No commands were sent to change the screen state - Assertions are not cleared after execution - This deviated from the pattern used in all other test cases
This comment has been minimized.
This comment has been minimized.
…d-test-assertion-7037 Background command test assertion
This comment has been minimized.
This comment has been minimized.
| jobNumberRegexp := regexp.MustCompile(`\[(\d+)\]\s+(\d+)`) | ||
| matches := jobNumberRegexp.FindStringSubmatch(outputText) | ||
|
|
||
| if len(matches) != 3 { |
There was a problem hiding this comment.
@UdeshyaDhungana what happens when the user prints an incorrect line (that doesn't match the regex?), I don't see that handled here. Also, we should probably verify the PID? That could be incorrect too.
There was a problem hiding this comment.
Added failure cases and updated test case to deliver correct error message.
| outputLine := asserter.Shell.GetScreenState().GetRow(asserter.GetLastLoggedRowIndex()) | ||
| outputText := outputLine.String() | ||
|
|
||
| // Keeping the capture group for PGID as well: we might need it later |
There was a problem hiding this comment.
@UdeshyaDhungana is this needed for now? It makes our assertions a bit less strict, unless it's a valid case for our current tests I'd remove
There was a problem hiding this comment.
I removed the PGID capture group. We won't need it for this extension.
| } | ||
|
|
||
| // TODO: Remove after PR review: The regex here complies with the Bash's implementation of 'jobs' | ||
| // Should I add the pattern compatible with ZSH's output as well? |
There was a problem hiding this comment.
Depends, what are the the differences @UdeshyaDhungana?
There was a problem hiding this comment.
@rohitpaulk Here is a short comparison:
ZSH:
[1] running sleep 20
[2] - running sleep 300
[3] + running sleep 40Bash
[1] Running sleep 200 &
[2]- Running sleep 400 &
[3]+ Running sleep 500 &Differences:
- ZSH uses a space separator between job number and marker (+/-)
- ZSH uses lowercase for job status (eg. running, done)
- Bash always shows an
&symbol for running jobs, zsh drops it
I've changed the regex to match both of these shell's outputs.
There was a problem hiding this comment.
Yep, sounds good! I think it's worth allowing both
| ) | ||
|
|
||
| allLinesAssertions = append(allLinesAssertions, assertions.SingleLineAssertion{ | ||
| FallbackPatterns: []*regexp.Regexp{regex}, |
There was a problem hiding this comment.
If we have assertions without expectedOutput, not sure how the error message is going to look like. We don't expose regexes to users afaik, so I imagine this'd be confusing.
@UdeshyaDhungana let's add test cases for failures - I think these are pretty basic mistakes that we'd have caught if we explicitly thought about what happens when users make mistakes
There was a problem hiding this comment.
Added regex mismatch error messages with hints pointing to regex101 just like we do in grep tester.
…e cases; Make JobsBuiltinResponseTestCase to support both bash and zsh patterns
…orMessageForExpectedOutputMismatch
…bs_incorrect_output_format
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
internal/test_helpers/fixtures/background_jobs_incorrect_output_format
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Changed (\s)? to \s* in the jobs builtin regex pattern to handle ZSH's labeled job format which uses 2+ spaces before the +/- marker (e.g., '[2] - running' instead of '[2]- running').
This comment has been minimized.
This comment has been minimized.
…rmat-1863 Jobs regex ZSH format
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (3c7ad29063)diff --git a/internal/assertions/single_line_regex_assertion.go b/internal/assertions/single_line_regex_assertion.go
--- a/internal/assertions/single_line_regex_assertion.go
+++ b/internal/assertions/single_line_regex_assertion.go
@@ -27,7 +27,7 @@
patterns = append(patterns, pattern.String())
}
- return fmt.Sprintf("SingleLineAssertion (%q)", strings.Join(patterns, "\n"))
+ return fmt.Sprintf("SingleLineRegexAssertion (%q)", strings.Join(patterns, "\n"))
}
func (a SingleLineRegexAssertion) Run(screenState screen_state.ScreenState, startRowIndex int) (processedRowCount int, err *AssertionError) { |
…nspect-name-eaf1 Regex assertion inspect name
|
| [33m[tester::#AT7] [0m[91m^ Line does not match the expected regex.[0m | ||
| [33m[tester::#AT7] [0m[91m[31mReceived:[0m "[1]2551"[0m | ||
| [33m[tester::#AT7] [0m[91m[32mExpected line to match the following regex:[0m | ||
| [33m[tester::#AT7] [0m[91m[0m[32m\[\d+\]\s+\d+(Hint: https://regex101.com/?regex=%5C%5B%5Cd%2B%5C%5D%5Cs%2B%5Cd%2B&testString=%5B1%5D2551)[0m[0m |
There was a problem hiding this comment.
@UdeshyaDhungana I think this is going to be super confusing. Can we try just setting expected output to "[1] "?
Expected: "[1] <PID>"
Received: "[1]2551"
Let's not expose the regex.
| [33m[tester::#AT7] [0m[94mRunning ./your_program.sh[0m | ||
| [33m[your-program] [0m[0m$ sleep 100 &[0m | ||
| [33m[your-program] [0m[0m[0] 2652[0m | ||
| [33m[tester::#AT7] [0m[91mExpected job number to be 1, got 0[0m |
There was a problem hiding this comment.
Wonder if this could just use our existing format?
Expected: "[1] <PID>"
Received: "[0] 2652"
Might not be super obvious what job number is here, and I think our assertion could in fact be simpler and re-use the existing ones if we do this?
| return 0, &AssertionError{ | ||
| ErrorRowIndex: startRowIndex, | ||
| Message: fmt.Sprintf("Line does not match %s.\n%s", | ||
| english.PluralWord(len(a.ExpectedRegexPatterns), "the expected regex", "any of the expected regexes"), |
There was a problem hiding this comment.
Let's scrap this - we shouldn't expose regex to users, can be confusing. We should keep our expectations very human-level, just seeing the expected vs. received output should make it clear what's wrong and needs to be fixed
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unescaped regex metacharacter in job marker causes panic
- Wrapped the marker variable with regexp.QuoteMeta() to escape the '+' character that would cause an invalid regex pattern when CurrentJob is used.
Or push these changes by commenting:
@cursor push a79e15ad0f
Preview (a79e15ad0f)
diff --git a/internal/test_cases/jobs_builtin_response_test_case.go b/internal/test_cases/jobs_builtin_response_test_case.go
--- a/internal/test_cases/jobs_builtin_response_test_case.go
+++ b/internal/test_cases/jobs_builtin_response_test_case.go
@@ -61,7 +61,7 @@
regexString := fmt.Sprintf(
`^\[%d\]\s*%s\s+(?i)%s\s+(?-i)%s`,
outputEntry.JobNumber,
- marker,
+ regexp.QuoteMeta(marker),
regexp.QuoteMeta(outputEntry.Status),
regexp.QuoteMeta(outputEntry.LaunchCommand),
)| regexString := fmt.Sprintf( | ||
| `^\[%d\]\s*%s\s+(?i)%s\s+(?-i)%s`, | ||
| outputEntry.JobNumber, | ||
| marker, |
There was a problem hiding this comment.
Unescaped regex metacharacter in job marker causes panic
High Severity
The marker value from convertJobMarkerToString is interpolated directly into the regex pattern on line 62 without regexp.QuoteMeta(). When CurrentJob is used, marker is "+", producing the pattern \s*+ in the regex string. Go's RE2 engine does not support possessive quantifiers, so regexp.MustCompile on line 77 will panic at runtime. The Status and LaunchCommand fields are properly escaped but marker is not.
Additional Locations (1)
There was a problem hiding this comment.
Please ignore this. I've changed this in the next PR where this is actually used, including error cases.
rohitpaulk
left a comment
There was a problem hiding this comment.
Looks good but we should add in the PID check too (can be another PR)
| [33m[your-program] [0m[0m[0] 2233[0m | ||
| [33m[tester::#AT7] [0m[91m^ Line does not match expected value.[0m | ||
| [33m[tester::#AT7] [0m[91m[32mExpected:[0m "[1] <PID>"[0m | ||
| [33m[tester::#AT7] [0m[91m[31mReceived:[0m "[0] 2233"[0m |
There was a problem hiding this comment.
@UdeshyaDhungana another test we should add: invalid PID. A user could print 1234 for the PID and pass at the moment
There was a problem hiding this comment.
Sure. Will raise a new PR.




Note
Medium Risk
Adds new stage slugs and assertion logic around background process output formatting, which could introduce flakiness across different shells/PTY behavior; otherwise changes are isolated to the tester and fixtures.
Overview
Adds a new Background Jobs extension with Stage
af3andat7, including new stage handlers (testBG1/testBG2) and test-case helpers to validatejobsbeing registered as a builtin,jobsoutput (including empty output), and background launches viacmd &emitting"[job] PID"and returning to the prompt.Wires the new stages into the tester definition, Makefile test targets (including
test_background_jobs_w_bash), and course metadata, and expands fixtures/scenarios to cover common failure modes (bad output format/job number) plus output normalization for variable job/PID values. Updates local docker test runner modes (addstests_excluding_ash, allows defaulttestwith ash) and refactorsBuildColoredErrorMessageparameter naming/usage;go.mod/go.sumpick upgo-humanizeindirectly.Written by Cursor Bugbot for commit 7356938. This will update automatically on new commits. Configure here.