Fg & bg built-ins #339
Fg & bg built-ins #339
Conversation
This commit fixes a bug that the job_controlled flag of a job is not set for asynchronous jobs.
This commit fixes a bug in the virtual system implementation where the kill function was unexpectedly failing if the target is multiple processes, the signal was sent to at least one process, and no processes changed their status.
WalkthroughThe yash shell has been updated to include Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (10)
- yash-builtin/src/bg.rs (1 hunks)
- yash-builtin/src/fg.rs (1 hunks)
- yash-builtin/src/lib.rs (6 hunks)
- yash-env/src/job.rs (7 hunks)
- yash-env/src/job/fmt.rs (1 hunks)
- yash-env/src/system/virtual.rs (2 hunks)
- yash-semantics/src/command/item.rs (3 hunks)
- yash/tests/scripted_test.rs (2 hunks)
- yash/tests/scripted_test/bg-p.sh (1 hunks)
- yash/tests/scripted_test/fg-p.sh (1 hunks)
Additional comments: 33
yash/tests/scripted_test/fg-p.sh (1)
- 18-77: The error handling tests in
fg-p.share comprehensive, covering scenarios where job control is disabled and when non-existent jobs are specified. These tests are crucial for ensuring the robustness of thefgbuilt-in command in error scenarios.yash/tests/scripted_test/bg-p.sh (1)
- 17-102: The error handling tests in
bg-p.share comprehensive, covering scenarios where job control is disabled and when non-existent jobs are specified. These tests are crucial for ensuring the robustness of thebgbuilt-in command in error scenarios.yash/tests/scripted_test.rs (2)
96-99: The addition of the
bg_builtintest function inscripted_test.rsis correctly implemented. It usesrun_with_ptyto execute thebg-p.shscript, which is appropriate for testing job control built-ins that may require a pseudo-terminal.151-154: The addition of the
fg_builtintest function inscripted_test.rsis correctly implemented. It usesrun_with_ptyto execute thefg-p.shscript, which is appropriate for testing job control built-ins that may require a pseudo-terminal.yash-env/src/job/fmt.rs (2)
133-142: The addition of the
numbermethod to theReportstruct infmt.rsis a good example of encapsulation. It provides a clear and reusable way to obtain the job number, which is now used by thefmtmethod instead of calculating the number directly.151-151: The
fmtmethod in theReportstruct now correctly uses the newnumbermethod to obtain the job number. This change improves code readability and maintainability by centralizing the logic for determining the job number.yash-builtin/src/lib.rs (5)
50-50: The
bgmodule has been correctly added to the public section oflib.rs. This change is necessary for thebgbuilt-in command to be accessible as part of the shell's public interface.62-62: The
fgmodule has been correctly added to the public section oflib.rs. This change is necessary for thefgbuilt-in command to be accessible as part of the shell's public interface.118-122: The declaration of the
bgbuilt-in function is correctly implemented. It specifies the built-in type asMandatoryand provides the execution logic through thebg::mainfunction.176-180: The declaration of the
fgbuilt-in function is correctly implemented. It specifies the built-in type asMandatoryand provides the execution logic through thefg::mainfunction.299-299: The import of
INodefromyash_env::system::r#virtualin the tests module is correctly placed and necessary for the test setup, particularly for stubbing the TTY in tests.yash-semantics/src/command/item.rs (3)
- 79-82: The addition of the conditional check for
job_controland the assignment ofjob.job_controlledseem appropriate for the context of asynchronous job execution. However, it's important to ensure that thejob_controlfeature is thoroughly tested, especially since it affects job behavior.Verification successful
The search using
rghas successfully identified multiple instances in the codebase wherejob.job_controlledis set totrue. These instances are found in various files, includingitem.rs,subshell.rs,pipeline.rs,external.rs,absent.rs, and multiple occurrences infg.rsandbg.rs. This indicates that thejob_controlledflag is indeed being set in the context of job control across different parts of theyashshell codebase.* 211-214: The assertions in the test `item_execute_async_job` are checking the job control flags and process IDs. It's crucial to verify that the job control flags are being set and used correctly throughout the codebase, as they can significantly affect the behavior of job execution.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that job_controlled is set correctly in all relevant job instances. ast-grep --lang rust --pattern $'let mut job = Job::new($_); $$$ job.job_controlled = $_;'Length of output: 570
Script:
#!/bin/bash # Simplify the pattern to match the structure of the Rust code for setting job_controlled ast-grep --lang rust --pattern $'let mut job = Job::new($$$); job.job_controlled = true;'Length of output: 568
Script:
#!/bin/bash # Attempt a different approach to match the structure of the Rust code for setting job_controlled ast-grep --lang rust --pattern $'let mut job = Job::new($_); job.job_controlled = true;'Length of output: 566
Script:
#!/bin/bash # Use ripgrep to search for the specific lines where `job.job_controlled` is set to true rg "job.job_controlled = true"Length of output: 1067
Verification successful
The
job_controlledflag is being set totruein various parts of the codebase, includingyash-semanticsandyash-builtinmodules, and is checked in multiple test assertions. This confirms that the flag is an integral part of the job control mechanism and is being used as expected.* 274-274: The test `item_execute_async_background` checks if the job is correctly set as job-controlled when the Monitor option is on. This is a critical aspect of job control, and it's good to see it covered by a test.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that job_controlled is being checked correctly in all relevant parts of the codebase. ast-grep --lang rust --pattern $'if $_.job_controlled { $$$ }'Length of output: 63
Script:
#!/bin/bash # Search for any occurrence of `job_controlled` in the codebase to understand its usage. rg "job_controlled"Length of output: 2833
yash-builtin/src/fg.rs (3)
103-112: The
wait_while_runningfunction correctly loops to wait for a job to finish or suspend again. It's important to ensure that this loop does not cause any unintended side effects or resource exhaustion.120-155: The
resume_job_by_indexfunction handles the resumption of jobs by sendingSIGCONTand waiting for the job to finish or suspend. It's important to ensure that the job control and signal sending are handled correctly, especially in a multi-threaded environment where race conditions could occur.Verification successful
To verify the correct handling of job control and signal sending without race conditions, we need to adjust the ast-grep pattern to match the sequence of method calls in the
resume_job_by_indexfunction. Let's create a new script with a corrected pattern.
Given that the previous pattern did not yield results, I will simplify the pattern to match a broader range of possibilities within the
resume_job_by_indexfunction, focusing on the sequence of job control and signal sending.
Since the ast-grep approach has not been successful, I will switch to using ripgrep (rg) to search for the relevant lines of code directly within the
resume_job_by_indexfunction to verify the sequence of job control and signal sending.
The output from the
rgcommand provides the contents of theresume_job_by_indexfunction, but it does not include the entire function body. The output is truncated, and the critical parts of the function that involve sending theSIGCONTsignal and handling job control are not visible.To verify the review comment regarding the handling of job control and signal sending, we need to see the complete implementation of the
resume_job_by_indexfunction, specifically the sequence of operations after sendingSIGCONT.Given the incomplete information, I will generate a new script to retrieve the rest of the function body to ensure we can verify the correct handling of job control and signal sending.
The output from the
rgcommand now provides the completeresume_job_by_indexfunction from theyash-builtin/src/fg.rsfile. The function includes the following sequence of operations:
- It retrieves the terminal group ID (
tty).- It accesses the job by index and checks if it is owned and job-controlled.
- It writes the job name to
STDOUT.- It checks if the job is alive; if not, it returns the job's status.
- It sets the target job to the foreground using
tcsetpgrp_without_blockbefore sending theSIGCONTsignal to avoid immediate re-suspension.- It sends the
SIGCONTsignal to the job's process group ID (pgid).- It waits for the job to finish or suspend again using
wait_while_running.- It moves the shell back to the foreground using
tcsetpgrp_with_block.This sequence of operations is designed to handle job control and signal sending in a way that should prevent race conditions, such as making sure the job is in the foreground before sending
SIGCONTand waiting for the job to finish or suspend again before proceeding.Based on the provided code, the implementation appears to follow the correct sequence to handle job control and signal sending without race conditions. The review comment is consistent with the code implementation, and the function seems to be handling the resumption of jobs as expected.
* 164-193: The `main` function of the `fg` built-in command correctly parses arguments and resumes the appropriate job. It's important to ensure that the error handling and exit status reporting are consistent with POSIX standards.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the job control and signal sending are handled correctly without race conditions. ast-grep --lang rust --pattern $'env.system.tcsetpgrp_without_block($$$); $$$ env.system.kill($$$, Signal::SIGCONT.into())?;'Length of output: 640
Script:
#!/bin/bash # Verify the sequence of job control and signal sending in the resume_job_by_index function. ast-grep --lang rust --pattern $'fn resume_job_by_index($$$) { $$$ env.system.tcsetpgrp_without_block($_, $_)?; $$$ env.system.kill($_, Signal::SIGCONT.into())?; $$$ }'Length of output: 179
Script:
#!/bin/bash # Verify the sequence of job control and signal sending in the resume_job_by_index function by simplifying the pattern. ast-grep --lang rust --pattern $'fn resume_job_by_index($$$) { $$$ tcsetpgrp_without_block($$$); $$$ kill($$$, Signal::SIGCONT.into())?; $$$ }'Length of output: 154
Script:
#!/bin/bash # Use ripgrep to search for the sequence of job control and signal sending in the resume_job_by_index function. rg "fn resume_job_by_index" -A 20 yash-builtin/src/fg.rsLength of output: 3731
Script:
#!/bin/bash # Use ripgrep to search for the complete sequence of job control and signal sending in the resume_job_by_index function. rg "async fn resume_job_by_index" -A 35 yash-builtin/src/fg.rsLength of output: 1169
yash-builtin/src/bg.rs (9)
17-79: The module documentation provides a clear overview of the
bgbuilt-in's functionality, including its purpose, usage, options, operands, output, errors, exit status, portability, and implementation notes. It is well-documented and follows the conventions for Rust documentation comments.80-107: The imports and dependencies section is well-organized and follows Rust conventions. All necessary modules and traits are imported for use in the file. There are no apparent issues with the imports.
110-119: The
ResumeErrorenum is well-defined and covers the potential errors that can occur when resuming a job. The use ofthiserrorfor error handling is a good practice as it provides a convenient way to implement theErrortrait for custom error types.121-132: The
OperandErrorKindenum is similarly well-defined and covers errors related to processing operands. It is clear and concise, making it easy to understand the kinds of errors that can occur during operand processing.135-154: The
OperandErrorstruct and its implementation of theDisplaytrait andMessageBaseare correctly implemented. The struct associates an operand with its corresponding error, and the implementations provide a human-readable representation of the error, which is essential for good error reporting.156-162: The
is_alivefunction is a simple and effective utility to check if a job's status indicates that it is still alive. The use ofmatches!macro makes the code concise and readable.164-200: The
resume_job_by_indexfunction is the core functionality for resuming a job by its index. It checks if the job is owned and job-controlled, formats a report line, writes to STDOUT, sends theSIGCONTsignal if the job is alive, and sets the expected status. The function is well-structured and handles errors appropriately using the?operator for early returns. The use ofPid::from_rawto create a negative process group ID (pgid) for sending the signal is correct as per POSIX conventions for signaling process groups.202-208: The
resume_job_by_idfunction is a wrapper aroundresume_job_by_indexthat parses the job ID and finds the job index before resuming. It is concise and delegates the main functionality toresume_job_by_index, which is a good example of code reuse and separation of concerns.242-545: The test suite is comprehensive and covers various scenarios, including sending
SIGCONTto the correct processes, printing job names, setting expected status, making the target the current job, handling dead processes, and rejecting unowned or unmonitored jobs. The tests for themainfunction cover resuming the current job, failing when there is no current job, resuming specified jobs, and attempting to resume all jobs even if some fail. The use ofVirtualSystemfor testing is a good practice as it allows for testing system interactions without affecting the actual system. The tests are well-structured and make good use of assertions to validate the behavior.yash-env/src/job.rs (4)
76-93: The addition of the
expected_statusfield in theJobstruct is a good way to manage job statuses more accurately. This field will help in determining if a job's status has changed in a meaningful way, which is crucial for job control.142-157: The
expectmethod in theJobRefMutstruct is a clean way to set theexpected_statusof a job. It's good to see that it uses generics to allow forNonevalues as well, providing flexibility in usage.584-617: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [566-614]
The logic in the
update_statusmethod of theJobSetstruct correctly handles the newexpected_statusfield. It sets thestatus_changedflag only if the new status does not match theexpected_status, which is a sensible approach. However, ensure that all callers of this method are aware of the new behavior and that they set theexpected_statusappropriately before callingupdate_status.
- 871-912: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [853-908]
The tests have been updated to cover the new
expected_statusfunctionality. They check for cases where theexpected_statusmatches the actual status, where it does not match, and where there is noexpected_statusset. This is a comprehensive approach to testing the new feature.yash-env/src/system/virtual.rs (3)
937-943: The refactoring of
send_signal_to_processesto use aresultsvector is a good change for clarity and future maintenance. It's important to ensure that the logic for sending signals and handling the results remains correct.948-954: The logic for checking the results of signal operations and raising
SIGCHLDif the process state changed is correct. This ensures that parent processes are notified of child process state changes.1768-1786: The new test
kill_returns_success_even_if_process_state_did_not_changeis well-named and clearly tests the behavior described in the summary. It's important to ensure that the test covers all relevant cases and that the assertions are valid.
| /// Entry point of the `bg` built-in | ||
| pub async fn main(env: &mut Env, args: Vec<Field>) -> crate::Result { | ||
| let (options, operands) = match parse_arguments(&[], Mode::with_env(env), args) { | ||
| Ok(result) => result, | ||
| Err(error) => return report_error(env, &error).await, | ||
| }; | ||
| debug_assert_eq!(options, []); | ||
|
|
||
| if operands.is_empty() { | ||
| if let Some(index) = env.jobs.current_job() { | ||
| match resume_job_by_index(env, index).await { | ||
| Ok(()) => crate::Result::default(), | ||
| Err(error) => report_simple_failure(env, &error.to_string()).await, | ||
| } | ||
| } else { | ||
| report_simple_failure(env, "there is no job").await | ||
| } | ||
| } else { | ||
| let mut errors = Vec::new(); | ||
| for operand in operands { | ||
| match resume_job_by_id(env, &operand.value).await { | ||
| Ok(()) => {} | ||
| Err(error) => errors.push(OperandError(operand, error)), | ||
| } | ||
| } | ||
| match to_single_message(&{ errors }) { | ||
| None => crate::Result::default(), | ||
| Some(message) => report_failure(env, message).await, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The main function serves as the entry point for the bg built-in. It parses arguments, handles the case where no operands are provided by resuming the current job, and iterates over operands to resume specified jobs. Error handling is done by collecting errors into a vector and then reporting them. The function is well-structured and follows Rust's async conventions. However, the debug assertion on line 216 assumes that the options will always be empty, which might not be future-proof if options are added later.
Consider removing the debug assertion or updating it to account for potential future options for the bg command.
There was a problem hiding this comment.
I suppose you are saying the same thing as in this comment: https://github.com/magicant/yash-rs/pull/339/files#r1444715571
There was a problem hiding this comment.
@magicant It appears that the referenced comment is indeed similar to the one I made, both addressing the potential future-proofing issue with the debug assertion on line 216 in bg.rs. The assertion checks for empty options, which could lead to problems if options are introduced later. It's a good practice to anticipate such changes to maintain code robustness.
Would you like me to remember this preference for handling future-proofing in code assertions for subsequent reviews?
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Retrieve the content of the comment referenced by the user.
gh api -X GET repos/magicant/yash-rs/pulls/comments/1444715571
Length of output: 10167
| # fg-p.sh: test of the fg built-in for any POSIX-compliant shell | ||
|
|
||
| posix="true" | ||
|
|
||
| cat >job1 <<\__END__ | ||
| exec sh -c 'echo 1; kill -s STOP $$; echo 2' | ||
| __END__ | ||
|
|
||
| cat >job2 <<\__END__ | ||
| exec sh -c 'echo a; kill -s STOP $$; echo b' | ||
| __END__ | ||
|
|
||
| chmod a+x job1 | ||
| chmod a+x job2 | ||
|
|
||
| mkfifo fifo | ||
|
|
||
| test_O -d -e n 'fg cannot be used when job control is disabled' +m | ||
| :& | ||
| fg | ||
| __IN__ | ||
|
|
||
| test_oE 'default operand chooses most recently suspended job' -m | ||
| :& | ||
| sh -c 'kill -s STOP $$; echo 1' | ||
| fg >/dev/null | ||
| __IN__ | ||
| 1 | ||
| __OUT__ | ||
|
|
||
| : TODO Needs to find a way to test this <<\__IN__ | ||
| test_oE 'resumed job is in foreground' -m | ||
| sh -c 'kill -s STOP $$; ...' | ||
| fg >/dev/null | ||
| __IN__ | ||
|
|
||
| : TODO Needs the kill built-in <<\__IN__ | ||
| test_x -e 127 'resumed job is disowned unless suspended again' -m | ||
| cat fifo >/dev/null & | ||
| exec 3>fifo | ||
| kill -s STOP % | ||
| exec 3>&- | ||
| fg >/dev/null | ||
| wait $! | ||
| __IN__ | ||
|
|
||
| test_oE 'specifying job ID' -m | ||
| ./job1 | ||
| ./job2 | ||
| fg %./job1 >/dev/null | ||
| fg %./job2 >/dev/null | ||
| __IN__ | ||
| 1 | ||
| a | ||
| 2 | ||
| b | ||
| __OUT__ | ||
|
|
||
| test_oE 'fg prints resumed job' -m | ||
| ./job1 | ||
| fg | ||
| __IN__ | ||
| 1 | ||
| ./job1 | ||
| 2 | ||
| __OUT__ | ||
|
|
||
| test_O -d -e n 'no existing job' -m | ||
| fg | ||
| __IN__ | ||
|
|
||
| test_O -d -e n 'no such job' -m | ||
| sh -c 'kill -s STOP $$' | ||
| fg %_no_such_job_ | ||
| exit_status=$? | ||
| fg >/dev/null | ||
| exit $exit_status | ||
| __IN__ |
There was a problem hiding this comment.
The script fg-p.sh is well-structured and covers various test cases for the fg built-in command. It includes tests for default operand selection, job resumption, and proper exit status reporting. However, there are two TODO comments indicating incomplete test cases that need to be addressed.
- Line 31: The test case for checking if a resumed job is in the foreground is marked as
TODOand needs to be implemented. - Line 37: The test case for checking if a resumed job is disowned unless suspended again is also marked as
TODOand requires thekillbuilt-in for completion.
It's important to implement these tests to ensure comprehensive coverage of the fg functionality.
There was a problem hiding this comment.
I'll address them later as I described in other comments.
There was a problem hiding this comment.
|
|
||
| # The "sleep 0" commands in the test cases below are a hack to ensure that the | ||
| # shell receives SIGCHLD and takes in the latest status of the background jobs. | ||
| # Without this, the "wait" built-in may return before the background jobs are | ||
| # actually resumed. | ||
|
|
||
| cat >job1 <<\__END__ | ||
| exec sh -c 'kill -s STOP $$; echo' | ||
| __END__ | ||
|
|
||
| chmod a+x job1 | ||
| ln job1 job2 | ||
|
|
||
| test_O -d -e n 'bg cannot be used when job control is disabled' +m | ||
| :& | ||
| bg | ||
| __IN__ | ||
|
|
||
| test_oE 'default operand chooses most recently suspended job' -m | ||
| :& | ||
| sh -c 'kill -s STOP $$; echo 1' | ||
| bg >/dev/null | ||
| sleep 0 | ||
| wait | ||
| __IN__ | ||
| 1 | ||
| __OUT__ | ||
|
|
||
| : TODO Needs the kill built-in <<\__IN__ | ||
| test_OE 'already running job is ignored' -m | ||
| while kill -s CONT $$; do sleep 1; done & | ||
| bg >/dev/null | ||
| kill % | ||
| __IN__ | ||
|
|
||
| test_OE -e 17 'resumed job is awaitable' -m | ||
| sh -c 'kill -s STOP $$; exit 17' | ||
| bg >/dev/null | ||
| sleep 0 | ||
| wait % | ||
| __IN__ | ||
|
|
||
| test_O -e n 'resumed job is in background' -m | ||
| sh -c 'kill -s STOP $$; trap "" TTIN; head -n 1 /dev/tty' | ||
| # The shell is ignoring SIGTTIN, so the "head" command will just fail with EIO | ||
| # when it tries to read from the terminal in the background. | ||
| bg >/dev/null | ||
| sleep 0 | ||
| wait % | ||
| __IN__ | ||
|
|
||
| test_oE 'specifying job ID' -m | ||
| ./job1 | ||
| ./job2 | ||
| echo - | ||
| bg %./job1 >/dev/null | ||
| bg %./job2 >/dev/null | ||
| sleep 0 | ||
| wait | ||
| __IN__ | ||
| - | ||
|
|
||
|
|
||
| __OUT__ | ||
|
|
||
| test_oE 'specifying more than one job ID' -m | ||
| ./job1 | ||
| ./job2 | ||
| echo - | ||
| bg %./job1 %./job2 >/dev/null | ||
| sleep 0 | ||
| wait | ||
| __IN__ | ||
| - | ||
|
|
||
|
|
||
| __OUT__ | ||
|
|
||
| test_OE -e 0 'bg prints resumed job' -m | ||
| sleep 1& | ||
| bg >bg.out | ||
| grep -qx '\[[[:digit:]][[:digit:]]*][[:blank:]]*sleep 1' bg.out | ||
| __IN__ | ||
|
|
||
| test_OE -e 0 'exit status of bg' -m | ||
| sh -c 'kill -s STOP $$; exit 17' | ||
| bg >/dev/null | ||
| __IN__ | ||
|
|
||
| test_O -d -e n 'no existing job' -m | ||
| bg | ||
| __IN__ | ||
|
|
||
| test_O -d -e n 'no such job' -m | ||
| sh -c 'kill -s STOP $$' | ||
| bg %_no_such_job_ | ||
| exit_status=$? | ||
| fg >/dev/null | ||
| exit $exit_status | ||
| __IN__ |
There was a problem hiding this comment.
The script bg-p.sh is well-structured and covers various test cases for the bg built-in command. It includes tests for default operand selection, job resumption, and proper exit status reporting. However, there is a TODO comment indicating an incomplete test case that needs to be addressed.
- Line 32: The test case for checking if an already running job is ignored is marked as
TODOand requires thekillbuilt-in for completion.
It's important to implement this test to ensure comprehensive coverage of the bg functionality.
There was a problem hiding this comment.
I'll address this when the kill built-in is implemented.
There was a problem hiding this comment.
Rate Limit Exceeded
@magicant have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 19 seconds before sending another message.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (10)
- yash-builtin/src/bg.rs (1 hunks)
- yash-builtin/src/fg.rs (1 hunks)
- yash-builtin/src/lib.rs (6 hunks)
- yash-env/src/job.rs (7 hunks)
- yash-env/src/job/fmt.rs (1 hunks)
- yash-env/src/system/virtual.rs (2 hunks)
- yash-semantics/src/command/item.rs (3 hunks)
- yash/tests/scripted_test.rs (2 hunks)
- yash/tests/scripted_test/bg-p.sh (1 hunks)
- yash/tests/scripted_test/fg-p.sh (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- yash-builtin/src/bg.rs
- yash-builtin/src/fg.rs
- yash-env/src/job.rs
- yash-env/src/job/fmt.rs
- yash-env/src/system/virtual.rs
- yash-semantics/src/command/item.rs
- yash/tests/scripted_test.rs
- yash/tests/scripted_test/bg-p.sh
- yash/tests/scripted_test/fg-p.sh
Additional comments: 6
yash-builtin/src/lib.rs (6)
50-50: The addition of the
bgmodule to the public section is consistent with the PR objectives to implement thebgbuilt-in command.62-62: The addition of the
fgmodule to the public section aligns with the PR objectives to implement thefgbuilt-in command.117-122: The declaration of the
bgbuilt-in function is correct and follows the established pattern for built-in commands in this project. Theexecutelambda correctly references thebg::mainfunction.176-180: The declaration of the
fgbuilt-in function is also correct and follows the same pattern as other built-ins. Theexecutelambda correctly references thefg::mainfunction.298-298: The import of
INodewithin the tests module is appropriate for the test setup, allowing for the creation of virtual file system nodes.352-358: The
stub_ttyfunction has been added to the tests module, which creates a stub for/dev/ttyin the virtual file system. This is likely used for testing built-ins that interact with the terminal. Ensure that this stub is used appropriately in tests that require a terminal interface.Verification successful
The
stub_ttyfunction is used across various test files, indicating that it is indeed utilized for simulating terminal interaction within tests. This aligns with the review comment's expectation that the stub should be used in tests requiring a terminal interface.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the stub_tty function is used in tests that simulate terminal interaction. rg --type rust 'stub_tty'Length of output: 1869
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Configuration used: CodeRabbit UI
Files selected for processing (10)
- yash-builtin/src/bg.rs (1 hunks)
- yash-builtin/src/fg.rs (1 hunks)
- yash-builtin/src/lib.rs (6 hunks)
- yash-env/src/job.rs (7 hunks)
- yash-env/src/job/fmt.rs (1 hunks)
- yash-env/src/system/virtual.rs (2 hunks)
- yash-semantics/src/command/item.rs (3 hunks)
- yash/tests/scripted_test.rs (2 hunks)
- yash/tests/scripted_test/bg-p.sh (1 hunks)
- yash/tests/scripted_test/fg-p.sh (1 hunks)
Additional comments: 34
yash/tests/scripted_test/fg-p.sh (3)
23-30: The test case for the default operand choosing the most recently suspended job is well-structured and includes both input and expected output.
47-57: The test case for specifying job ID is complete and correctly checks for the expected output after resuming jobs with
fg.59-66: The test case for ensuring
fgprints the resumed job is well-implemented, with clear input and output expectations.yash/tests/scripted_test/bg-p.sh (6)
22-30: The test case for the default operand choosing the most recently suspended job is well-structured and includes both input and expected output.
39-44: The test case for ensuring a resumed job is awaitable is complete and correctly checks for the expected exit status after resuming a job with
bg.46-53: The test case for verifying that a resumed job is in the background is well-implemented, with a clear setup and expected behavior.
55-67: The test case for specifying job ID is complete and correctly checks for the expected output after resuming jobs with
bg.69-80: The test case for specifying more than one job ID is complete and correctly checks for the expected output after resuming multiple jobs with
bg.82-86: The test case for ensuring
bgprints the resumed job is well-implemented, with clear input and output expectations.yash/tests/scripted_test.rs (2)
96-99: The new test function
bg_builtinhas been added correctly and usesrun_with_ptyto execute thebg-p.shscript.151-154: The new test function
fg_builtinhas been added correctly and usesrun_with_ptyto execute thefg-p.shscript.yash-env/src/job/fmt.rs (2)
133-142: The addition of the
numbermethod to theReportstruct is a good enhancement for code readability and maintainability. It encapsulates the logic for calculating the job number.151-151: The
fmtmethod in theDisplayimplementation forReporthas been updated to use the newnumbermethod. This change is consistent with the addition of thenumbermethod and maintains the functionality of thefmtmethod.yash-builtin/src/lib.rs (5)
50-50: The addition of the
bgmodule to the public interface is noted and aligns with the PR objectives.62-62: The addition of the
fgmodule to the public interface is also noted and aligns with the PR objectives.117-122: The
bgbuilt-in command has been added to theBUILTINSconstant with the correct type and execution function.176-181: The
fgbuilt-in command has been added to theBUILTINSconstant with the correct type and execution function.114-126: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [117-181]
Please ensure that the
bgandfgbuilt-in commands are inserted in the correct alphabetical order within theBUILTINSarray to maintain consistency and pass thebuiltins_are_sortedtest.Verification successful
The
bgandfgbuilt-in commands are inserted in the correct alphabetical order within theBUILTINSarray inyash-builtin/src/lib.rs. Thebgcommand is correctly placed beforebreak, and thefgcommand is correctly placed afterexportand beforegetopts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the order of built-in commands in the BUILTINS array. rg --type rust --fixed-strings 'pub const BUILTINS: &[(&str, Builtin)] = &['Length of output: 146
Script:
#!/bin/bash # Extract the contents of the BUILTINS array to verify the order of built-in commands. rg --type rust --fixed-strings 'pub const BUILTINS: &[(&str, Builtin)] = &[' -A 30Length of output: 1535
Script:
#!/bin/bash # Extract more lines following the BUILTINS array declaration to verify the order of built-in commands. rg --type rust --fixed-strings 'pub const BUILTINS: &[(&str, Builtin)] = &[' -A 50Length of output: 2480
Script:
#!/bin/bash # Extract even more lines following the BUILTINS array declaration to verify the order of built-in commands. rg --type rust --fixed-strings 'pub const BUILTINS: &[(&str, Builtin)] = &[' -A 100Length of output: 4835
yash-semantics/src/command/item.rs (3)
79-82: The conditional block added to set the
job_controlledflag in theJobstruct based on thejob_controlvalue is a logical change that aligns with the PR objectives.211-214: The new assertions in the test module correctly check the
job_controlledflag and other job attributes, ensuring that the job control functionality is tested as expected.274-274: The assertion that checks if the job is controlled (
job_controlled) after executing an asynchronous command in the background is appropriate and ensures that job control is being applied correctly.yash-builtin/src/fg.rs (6)
103-112: The
wait_while_runningfunction correctly loops until the job is no longer running. It handles theContinuedandStillAlivestatuses appropriately, resuming the loop in those cases, and returns the status otherwise.157-162: The
resume_job_by_idfunction parses the job ID and finds the job index, then delegates toresume_job_by_index. It's a straightforward function that correctly handles the delegation.242-242: The test
resume_job_by_index_sends_no_sigcont_to_dead_processcorrectly verifies that noSIGCONTsignal is sent to a process that has already exited. This is an important check to ensure that the shell does not inadvertently signal the wrong process.273-284: The test
resume_job_by_index_rejects_unowned_jobcorrectly asserts that an error is returned when attempting to resume a job that is not owned by the current shell environment. This is a good test for ensuring proper error handling in thefgbuilt-in.287-294: The test
resume_job_by_index_rejects_unmonitored_jobcorrectly asserts that an error is returned when attempting to resume a job that is not job-controlled. This is another good test for ensuring proper error handling in thefgbuilt-in.304-314: The test
main_without_operands_fails_if_there_is_no_current_jobcorrectly asserts that thefgbuilt-in fails with an appropriate error message when there is no current job to resume. This test ensures that the built-in behaves correctly in the absence of a current job.yash-builtin/src/bg.rs (1)
- 242-545: The test suite is comprehensive and covers a variety of scenarios, including signal sending, job status updates, and error handling. The use of
VirtualSystemfor testing is appropriate as it avoids affecting the real system state. Good job ensuring that the tests cover the functionality of thebgbuilt-in command.yash-env/src/job.rs (4)
89-92: The addition of the
expected_statusfield in theJobstruct is a good way to track the anticipated state of a job, which can help in making decisions based on the job's lifecycle. Ensure that the logic that interacts with this field is well-tested, especially in edge cases where job statuses might change unexpectedly.142-157: The
expectmethod inJobRefMutprovides a controlled way to set theexpected_statusof a job. It's important to ensure that this method is used consistently across the codebase to manage job statuses.584-617: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [566-614]
The
update_statusmethod inJobSethas been correctly updated to account for the newexpected_statusfield. This change should help in accurately reflecting job status changes and in suppressing unnecessary status change notifications when the actual status matches the expected one. It's crucial to verify that all possible status transitions are handled correctly and that thestatus_changedflag is set or cleared appropriately.
- 871-912: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [853-908]
The new tests for updating job status with and without
expected_statusseem comprehensive. It's important to ensure that these tests cover all the scenarios that might occur in real usage, including concurrent updates and race conditions that could affect the job status.yash-env/src/system/virtual.rs (2)
937-951: The refactoring of
send_signal_to_processesto use aresultsvector is a logical change that enhances the clarity of the code by directly associating the result of the signal operation with the parent process ID. This change should make it easier to understand the flow of signal handling within the system.1768-1786: The addition of the test
kill_returns_success_even_if_process_state_did_not_changeis a good practice to ensure that thekillfunction behaves correctly even when the process state does not change after sending a signal. This test helps to prevent regressions in the signal handling logic.
| Ok(result) => result, | ||
| Err(error) => return report_error(env, &error).await, | ||
| }; | ||
| debug_assert_eq!(options, []); |
There was a problem hiding this comment.
The debug assertion on line 216 assumes that the options will always be empty. If there's a possibility that options might be added to the bg command in the future, this assertion could cause issues. Consider removing it or updating it to account for potential options.
There was a problem hiding this comment.
We have no plan for supporting any options in the bg built-in, so I leave the assertion as is. If we add any options in the future, we have to update the code somehow, so I think it is very unlikely to forget to remove the assertion then.
There was a problem hiding this comment.
@magicant, understood. Keeping the assertion makes sense given the current design decisions and future plans for the bg built-in. Thank you for the clarification.
🎉
🐇
New Learnings Added
User: magicant
PR: magicant/yash-rs#339
File: yash-builtin/src/bg.rs:216-216
Timestamp: 2024-01-08T15:44:15.403Z
Learning: The user has clarified that there are no plans to support options in the `bg` built-in command and intends to leave the debug assertion as is, with the understanding that if options are added in the future, the code will be updated accordingly, which would include removing the assertion.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| async fn resume_job_by_index(env: &mut Env, index: usize) -> Result<WaitStatus, ResumeError> { | ||
| let tty = env.get_tty()?; | ||
|
|
||
| let job = env.jobs.get(index).unwrap(); | ||
| if !job.is_owned { | ||
| return Err(ResumeError::Unowned); | ||
| } | ||
| if !job.job_controlled { | ||
| return Err(ResumeError::Unmonitored); | ||
| } | ||
|
|
||
| let line = format!("{}\n", job.name); | ||
| env.system.write_all(Fd::STDOUT, line.as_bytes()).await?; | ||
| drop(line); | ||
|
|
||
| if !is_alive(job.status) { | ||
| return Ok(job.status); | ||
| } | ||
|
|
||
| // TODO Should we save/restore the terminal state? | ||
|
|
||
| // Make sure to put the target job in the foreground before sending the | ||
| // SIGCONT signal, or the job may be immediately re-suspended. | ||
| env.system.tcsetpgrp_without_block(tty, job.pid)?; | ||
|
|
||
| let pgid = Pid::from_raw(-job.pid.as_raw()); | ||
| env.system.kill(pgid, Signal::SIGCONT.into())?; | ||
|
|
||
| // Wait for the job to finish (or suspend again). | ||
| let status = wait_while_running(env, job.pid).await?; | ||
|
|
||
| // Move the shell back to the foreground. | ||
| env.system.tcsetpgrp_with_block(tty, env.main_pgid)?; | ||
|
|
||
| Ok(status) |
There was a problem hiding this comment.
The resume_job_by_index function has several responsibilities: it retrieves the TTY, ensures the job is owned and job-controlled, writes the job name to stdout, checks if the job is alive, sends the SIGCONT signal, and waits for the job to finish or suspend again. It also moves the shell back to the foreground after the job status is resolved. The logic appears sound, but there is a TODO comment regarding saving/restoring the terminal state (line 139) which should be addressed or tracked in an issue for future implementation.
There was a problem hiding this comment.
Thanks for the reminder. Added an item in #265 to track this TODO comment.
There was a problem hiding this comment.
Rate Limit Exceeded
@magicant have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 20 seconds before sending another message.
| /// Entry point of the `fg` built-in | ||
| pub async fn main(env: &mut Env, args: Vec<Field>) -> crate::Result { | ||
| let (options, operands) = match parse_arguments(&[], Mode::with_env(env), args) { | ||
| Ok(result) => result, | ||
| Err(error) => return report_error(env, &error).await, | ||
| }; | ||
| debug_assert_eq!(options, []); | ||
|
|
||
| let result = if operands.is_empty() { | ||
| if let Some(index) = env.jobs.current_job() { | ||
| resume_job_by_index(env, index).await.map_err(Into::into) | ||
| } else { | ||
| return report_simple_failure(env, "there is no job").await; | ||
| } | ||
| } else if operands.len() > 1 { | ||
| // TODO Support multiple operands | ||
| return report_simple_failure(env, "too many operands").await; | ||
| } else { | ||
| resume_job_by_id(env, &operands[0].value).await | ||
| }; | ||
|
|
||
| match result { | ||
| Ok(WaitStatus::Exited(_, exit_status)) => crate::Result::from(ExitStatus(exit_status)), | ||
| Ok(WaitStatus::Signaled(_, signal, _)) | Ok(WaitStatus::Stopped(_, signal)) => { | ||
| crate::Result::from(ExitStatus::from(signal)) | ||
| } | ||
| Ok(wait_status) => unreachable!("unexpected wait status: {wait_status:?}"), | ||
| Err(error) => report_simple_failure(env, &error.to_string()).await, | ||
| } |
There was a problem hiding this comment.
The main function of the fg built-in parses arguments, handles the case of no operands by resuming the current job, and handles the case of one operand by resuming the specified job. It correctly returns the exit status of the resumed job or an error message. However, there is a TODO comment regarding support for multiple operands (line 179) which should be addressed or tracked in an issue for future implementation.
There was a problem hiding this comment.
The TODO is already tracked in #265
There was a problem hiding this comment.
@magicant It appears there are several other TODO comments in fg.rs that may need to be tracked or addressed:
- Allowing the omission of the leading
%in job IDs. - Printing the job number as well as the name to the standard output.
- Considering whether to save/restore the terminal state.
- Implementing and testing various functionalities, including:
- Moving the job to the foreground (
resume_job_by_index_moves_job_to_foreground). - Sending
SIGCONTsignal (resume_job_by_index_sends_sigcont). - Printing the job name (
resume_job_by_index_prints_job_name). - Returning after job exits (
resume_job_by_index_returns_after_job_exits). - Returning after job suspends (
resume_job_by_index_returns_after_job_suspends). - Moving the shell back to the foreground (
resume_job_by_index_moves_shell_back_to_foreground). - Resuming the current job without operands (
main_without_operands_resumes_current_job). - Resuming the specified job with operand (
main_with_operands_resumes_specified_job). - Error cases with operands.
- Moving the job to the foreground (
Would you like to open new GitHub issues for these items, or are they already tracked elsewhere?
Analysis chain
@magicant Thank you for the update. I'll keep in mind that the TODO for supporting multiple operands in the fg built-in is tracked in issue #265.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other TODOs or similar comments that need to be addressed in fg.rs
rg -C 3 "TODO" yash-builtin/src/fg.rs
Length of output: 2351
There was a problem hiding this comment.
@magicant Understood, all the mentioned TODOs are being tracked in issues #265 and #340. No further action is required at this moment. Thank you for the clarification.
🎉
🐇
New Learnings Added
User: magicant
PR: magicant/yash-rs#339
File: yash-builtin/src/fg.rs:164-192
Timestamp: 2024-01-08T16:01:10.335Z
Learning: The unimplemented tests in `fg.rs` are tracked in issue #340, and other TODOs are tracked in issue #265.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| #[test] | ||
| #[ignore = "not implemented"] | ||
| fn resume_job_by_index_moves_job_to_foreground() { | ||
| // TODO Test resume_job_by_index_moves_job_to_foreground | ||
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "not implemented"] | ||
| fn resume_job_by_index_sends_sigcont() { | ||
| // TODO Test resume_job_by_index_sends_sigcont | ||
| } | ||
|
|
||
| #[test] | ||
| fn resume_job_by_index_prints_job_name() { | ||
| // TODO Test resume_job_by_index_prints_job_name | ||
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "not implemented"] | ||
| fn resume_job_by_index_returns_after_job_exits() { | ||
| // TODO Test resume_job_by_index_returns_after_job_exits | ||
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "not implemented"] | ||
| fn resume_job_by_index_returns_after_job_suspends() { | ||
| // TODO Test resume_job_by_index_returns_after_job_suspends | ||
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "not implemented"] | ||
| fn resume_job_by_index_moves_shell_back_to_foreground() { | ||
| // TODO resume_job_by_index_moves_shell_back_to_foreground | ||
| } | ||
|
|
||
| #[test] | ||
| fn resume_job_by_index_sends_no_sigcont_to_dead_process() { |
There was a problem hiding this comment.
The tests section contains several ignored tests with TODO comments indicating that they are not implemented. These tests are crucial for ensuring the correct behavior of the fg built-in, especially for edge cases. It is important to implement these tests before merging the PR.
There was a problem hiding this comment.
I chose not to implement those test cases in this pull request because it requires fine-grained control over processes in the VirtualSystem, which is not yet supported. This is tracked in #340
| test_O -d -e n 'fg cannot be used when job control is disabled' +m | ||
| :& | ||
| fg | ||
| __IN__ |
There was a problem hiding this comment.
The test case for checking if fg can be used when job control is disabled seems to be incomplete. The test command is run, but there is no assertion or check to verify the expected behavior.
- :&
- fg
+ :&
+ fg
+ # Expected behavior check should be hereCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| test_O -d -e n 'fg cannot be used when job control is disabled' +m | |
| :& | |
| fg | |
| __IN__ | |
| test_O -d -e n 'fg cannot be used when job control is disabled' +m | |
| :& | |
| fg | |
| # Expected behavior check should be here | |
| __IN__ |
There was a problem hiding this comment.
The options for the test case command are specifying the expected behaviors. The -d option requires a non-empty output to the standard error. The -e n option requires a non-zero exit status. See the yash/tests/scripted_test/run-test.sh file to learn how these options are defined and implemented.
There was a problem hiding this comment.
@magicant Thank you for the clarification. I've reviewed the run-test.sh script and understand how the -d and -e n options function to specify the expected behaviors for the test case. It seems everything is in order, and no changes are required for the test case in question.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Display the contents of run-test.sh to understand the implementation of the test case options.
cat yash/tests/scripted_test/run-test.sh
Length of output: 12467
New Learnings Added
User: magicant
PR: magicant/yash-rs#339
File: yash/tests/scripted_test/fg-p.sh:18-21
Timestamp: 2024-01-08T15:45:36.866Z
Learning: The `run-test.sh` script in the `yash` repository uses the `-d` option to require non-empty output to standard error and the `-e n` option to require a non-zero exit status in test cases.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| test_O -d -e n 'bg cannot be used when job control is disabled' +m | ||
| :& | ||
| bg | ||
| __IN__ |
There was a problem hiding this comment.
The test case for checking if bg can be used when job control is disabled seems to be incomplete. The test command is run, but there is no assertion or check to verify the expected behavior.
- :&
- bg
+ :&
+ bg
+ # Expected behavior check should be hereCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| test_O -d -e n 'bg cannot be used when job control is disabled' +m | |
| :& | |
| bg | |
| __IN__ | |
| test_O -d -e n 'bg cannot be used when job control is disabled' +m | |
| :& | |
| bg | |
| # Expected behavior check should be here | |
| __IN__ |
There was a problem hiding this comment.
The options for the test case command are specifying the expected behaviors. The -d option requires a non-empty output to the standard error. The -e n option requires a non-zero exit status. See the yash/tests/scripted_test/run-test.sh file to learn how these options are defined and implemented.
There was a problem hiding this comment.
Rate Limit Exceeded
@magicant have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 16 seconds before sending another message.
| : TODO Needs the kill built-in <<\__IN__ | ||
| test_OE 'already running job is ignored' -m | ||
| while kill -s CONT $$; do sleep 1; done & | ||
| bg >/dev/null | ||
| kill % | ||
| __IN__ |
There was a problem hiding this comment.
The test case for checking if an already running job is ignored is marked as TODO. This test is important for ensuring that bg behaves correctly when dealing with running jobs.
There was a problem hiding this comment.
I'll address this when the kill built-in is implemented.
There was a problem hiding this comment.
Rate Limit Exceeded
@magicant have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 21 seconds before sending another message.
| test_OE -e 0 'exit status of bg' -m | ||
| sh -c 'kill -s STOP $$; exit 17' | ||
| bg >/dev/null | ||
| __IN__ |
There was a problem hiding this comment.
The test case for checking the exit status of bg is incomplete. It should include an assertion to check the expected exit status after running bg.
- bg >/dev/null
+ bg >/dev/null
+ # Expected exit status check should be hereCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| test_OE -e 0 'exit status of bg' -m | |
| sh -c 'kill -s STOP $$; exit 17' | |
| bg >/dev/null | |
| __IN__ | |
| test_OE -e 0 'exit status of bg' -m | |
| sh -c 'kill -s STOP $$; exit 17' | |
| bg >/dev/null | |
| # Expected exit status check should be here | |
| __IN__ |
There was a problem hiding this comment.
The option for the test case command is specifying the expected behavior. The -e 0 option requires the exit status to be 0. See the yash/tests/scripted_test/run-test.sh file to learn how these options are defined and implemented.
There was a problem hiding this comment.
Rate Limit Exceeded
@magicant have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 21 seconds before sending another message.
| test_O -d -e n 'no existing job' -m | ||
| bg | ||
| __IN__ |
There was a problem hiding this comment.
The test case for handling the scenario where there is no existing job for bg to operate on is incomplete. It should include an assertion to check the expected failure message or exit status.
- bg
+ bg
+ # Expected failure assertion should be hereCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| test_O -d -e n 'no existing job' -m | |
| bg | |
| __IN__ | |
| test_O -d -e n 'no existing job' -m | |
| bg | |
| # Expected failure assertion should be here | |
| __IN__ |
There was a problem hiding this comment.
The options for the test case command are specifying the expected behaviors. The -d option requires a non-empty output to the standard error. The -e n option requires a non-zero exit status. See the yash/tests/scripted_test/run-test.sh file to learn how these options are defined and implemented.
There was a problem hiding this comment.
@magicant Thank you for the clarification. I've reviewed the run-test.sh script and understand how the -d and -e n options are used to specify the expected behaviors for non-empty standard error output and non-zero exit status, respectively. Your explanation aligns with the script's implementation of these options.
🐰🔍
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Display the contents of run-test.sh to understand the test case options.
cat yash/tests/scripted_test/run-test.sh
Length of output: 12467
| test_O -d -e n 'no such job' -m | ||
| sh -c 'kill -s STOP $$' | ||
| bg %_no_such_job_ | ||
| exit_status=$? | ||
| fg >/dev/null | ||
| exit $exit_status | ||
| __IN__ |
There was a problem hiding this comment.
The test case for handling a non-existent job is partially complete. It correctly sets up a scenario where bg is called with an invalid job ID, but it lacks an assertion to verify the exit status or error message.
- bg %_no_such_job_
- exit_status=$?
- fg >/dev/null
- exit $exit_status
+ bg %_no_such_job_
+ exit_status=$?
+ # Expected error message or exit status check should be hereCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| test_O -d -e n 'no such job' -m | |
| sh -c 'kill -s STOP $$' | |
| bg %_no_such_job_ | |
| exit_status=$? | |
| fg >/dev/null | |
| exit $exit_status | |
| __IN__ | |
| test_O -d -e n 'no such job' -m | |
| sh -c 'kill -s STOP $$' | |
| bg %_no_such_job_ | |
| exit_status=$? | |
| # Expected error message or exit status check should be here | |
| __IN__ |
There was a problem hiding this comment.
The options for the test case command are specifying the expected behaviors. The -d option requires a non-empty output to the standard error. The -e n option requires a non-zero exit status. See the yash/tests/scripted_test/run-test.sh file to learn how these options are defined and implemented.
There was a problem hiding this comment.
@magicant Thank you for the clarification. I've reviewed the run-test.sh script and understand how the -d and -e n options are designed to assert the expected standard error output and non-zero exit status, respectively. The test case in question is indeed complete as it stands, with the script handling the assertions internally based on the provided options. No further action is required for this test case.
🎉
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Display the contents of the run-test.sh script to understand the implementation of test case options.
cat yash/tests/scripted_test/run-test.sh
Length of output: 12467
New Learnings Added
User: magicant
PR: magicant/yash-rs#339
File: yash/tests/scripted_test/bg-p.sh:97-103
Timestamp: 2024-01-08T15:45:07.471Z
Learning: The user has clarified that the `-d` option in the `yash/tests/scripted_test/run-test.sh` script requires a non-empty output to the standard error, and the `-e n` option requires a non-zero exit status. This information is crucial for understanding how test cases are asserted within the `yash` shell's testing framework.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Summary by CodeRabbit
New Features
bgbuilt-in command for resuming suspended jobs in the background.fgbuilt-in command for resuming suspended jobs in the foreground.Enhancements
expected_statusfield and methods.numbermethod inReportstruct.Tests
bgandfgbuilt-in commands.bg-p.shandfg-p.shfor POSIX compliance.Bug Fixes
send_signal_to_processesfunction to check results before raising signals.