Wait built-in: ignore disowned jobs & add scripted tests #335
Conversation
WalkthroughThe recent updates focus on enhancing job control within the shell environment. Changes include modifications to built-in commands for subshell behavior, treatment of disowned jobs, and improved job status tracking. Additionally, new fields and methods have been added to manage job ownership, and tests are now equipped with pseudo-terminal support for more accurate job control simulation. These updates aim to improve the reliability and predictability of job management in the shell. Changes
Poem
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? TipsChat with CodeRabbit Bot (
|
![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: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- Cargo.lock
- yash/Cargo.toml
Files selected for processing (8)
- yash-builtin/src/jobs.rs (1 hunks)
- yash-builtin/src/wait.rs (1 hunks)
- yash-builtin/src/wait/status.rs (3 hunks)
- yash-env/src/job.rs (4 hunks)
- yash-env/src/subshell.rs (3 hunks)
- yash/tests/pty/mod.rs (1 hunks)
- yash/tests/scripted_test.rs (3 hunks)
- yash/tests/scripted_test/wait-p.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- yash-builtin/src/wait.rs
Additional comments: 36
yash/tests/scripted_test/wait-p.sh (17)
5-8: The test case 'waiting for all jobs (+m)' seems to be correctly implemented, echoing characters to files in the background and then using
waitto synchronize before concatenating the files.15-17: The test case 'waiting for specific single job (+m)' is designed to wait for a specific job to finish using
$!, which is the PID of the last job run in the background. This is a standard way to test thewaitbuilt-in for a single job.20-24: The test case 'waiting for specific many jobs (+m)' correctly waits for multiple jobs by storing their PIDs in variables and then waiting on them. This is a good test for the
waitbuilt-in's ability to handle multiple job IDs.27-29: The test case 'waiting for unknown job (+m)' is testing the behavior of
waitwhen provided with an invalid job ID. The expected behavior is thatwaitshould return an exit status of 127 for the unknown job, which aligns with the PR objectives.32-35: The test case 'jobs are not inherited to subshells (+m, -s)' is checking that jobs started in the parent shell are not affecting the subshell, which is a key part of the PR objectives. The test seems to be correctly implemented.
38-39: The test case 'jobs are not inherited to subshells (+m, -c)' is similar to the previous one but uses
-cto run the shell command. It's important to ensure that this test is consistent with the behavior described in the PR objectives.42-45: The test case 'jobs are not propagated from subshells (+m)' is testing the opposite of the previous cases, ensuring that jobs started in a subshell do not affect the parent shell. This is another important aspect of the PR objectives.
48-51: The test case 'waiting for all jobs (-m)' is testing the
waitbuilt-in without job control enabled. This is a good test to ensure thatwaitbehaves correctly in different modes.58-60: The test case 'waiting for specific single job (-m)' is the non-job control mode equivalent of the earlier test for a single job. It's important to test both with and without job control.
63-67: The test case 'waiting for specific many jobs (-m)' is the non-job control mode equivalent of the earlier test for multiple jobs. It's good to see consistency in testing both modes.
70-72: The test case 'waiting for unknown job (-m)' is testing the
waitbuilt-in's behavior with an invalid job ID without job control. It should behave the same as in job control mode, returning an exit status of 127.75-79: The test case 'specifying job ID' is testing the
waitbuilt-in with job control string identifiers. This is a more advanced feature ofwaitand it's good to see it being tested.84-87: The test case 'jobs are not inherited to subshells (-m, -s)' is the non-job control mode equivalent of the earlier test for subshell job inheritance. It's important to ensure that the behavior is consistent across modes.
90-91: The test case 'jobs are not inherited to subshells (+m, -c)' seems to be a duplicate of the test on lines 38-39 but with a different mode specified. It's important to ensure that this is intentional and not a copy-paste error.
Verification successful
To verify the test case in question, I will first locate the
wait-p.shfile within the repository and then check for the presence of the test case 'jobs are not inherited to subshells (+m, -c)' to ensure it's not a duplicate.
The output from the script indicates that the test case 'jobs are not inherited to subshells (+m, -c)' appears twice in the
wait-p.shfile. This could suggest that it is indeed a duplicate, or it could be part of a larger test structure where the same description is used for different test scenarios.To further investigate, I will check the surrounding context of each occurrence to determine if they are testing different conditions or if it is a true duplicate.
The output shows two occurrences of the test case 'jobs are not inherited to subshells (+m, -c)' with a slight variation in the options passed to the test command. One occurrence uses the
-coption, while the other uses-cm. This indicates that the tests are intended to check the behavior of thewaitbuilt-in under different shell modes, and therefore, they are not exact duplicates but variations for different test scenarios.
- The first test is run with the
-coption only.- The second test is run with both
-cand-moptions.Given this context, it appears that the presence of both tests is intentional to verify the behavior of the
waitbuilt-in in different modes, and not a copy-paste error.* 94-97: The test case 'jobs are not propagated from subshells (-m)' is the non-job control mode equivalent of the earlier test for job propagation from subshells. It's good to see this aspect being tested in both modes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the test case 'jobs are not inherited to subshells (+m, -c)' is not a duplicate and is intended to test a different mode. grep -E 'jobs are not inherited to subshells \(\+m, -c\)' wait-p.sh | wc -lLength of output: 122
Script:
#!/bin/bash # Locate the wait-p.sh file and then check for the test case 'jobs are not inherited to subshells (+m, -c)' fd wait-p.sh | xargs grep -E 'jobs are not inherited to subshells \(\+m, -c\)'Length of output: 208
Script:
#!/bin/bash # Check the surrounding context of each occurrence of the test case 'jobs are not inherited to subshells (+m, -c)' fd wait-p.sh | xargs grep -E -C 3 'jobs are not inherited to subshells \(\+m, -c\)'Length of output: 335
100-101: The comment 'TODO Needs the kill built-in' indicates that there is a test case that is not yet implemented. It's important to track this TODO and ensure that the test is added before the PR is finalized.
101-116: The test case 'trap interrupts wait' is testing the behavior of
waitwhen an interrupt signal is received. This is a complex test case that involves signal handling and is crucial for ensuring the robustness of thewaitbuilt-in.yash/tests/pty/mod.rs (5)
35-50: The function
run_with_ptyis introduced to run a test subject in a pseudo-terminal, which is crucial for tests that depend on terminal-specific features like job control. The implementation seems to correctly handle the setup of the master and slave sides of the pseudo-terminal.53-58: The function
prepare_pty_masteris responsible for setting up the master side of the pseudo-terminal. It usesposix_openpt,grantpt, andunlockptwhich are standard functions for this purpose. The error handling withexpectis appropriate for a test environment where a failure should halt the test.64-67: The function
pty_slave_pathretrieves the path of the slave side of a pseudo-terminal. It uses a mutex to ensure thread safety around theptsnamefunction, which is not thread-safe. This is a good practice to prevent race conditions in a multi-threaded test environment.70-73: The function
open_pty_slaveopens the slave side of a pseudo-terminal. It uses theopensystem call with appropriate flags and error handling. The use ofOwnedFdensures proper ownership and cleanup of the file descriptor.80-100: The function
prepare_as_slaveprepares the slave side of a pseudo-terminal. It includes a comment about not allowing memory allocation or panicking because it's called in a child process, which is a critical consideration for forked processes. The implementation handles the different schemes for becoming the controlling process of a slave pseudo-terminal and checks for the race condition where an unrelated process could become the controlling process.yash/tests/scripted_test.rs (3)
33-59: The function
run_with_preexecis introduced to run a test subject with pre-execution setup. The use ofunsafeis justified due to the nature of the operations involved, such as working with file descriptors and process control. The implementation seems to correctly handle the setup and execution of the test subject.68-73: The function
runis refactored to userun_with_preexecinternally. This change simplifies therunfunction and centralizes the logic for running test subjects, which is a good practice for maintainability.262-265: The test function
wait_builtinis added to test the wait builtin feature. It usesrun_with_ptyto ensure that the test is run in a pseudo-terminal, which is necessary for testing job control features.yash-builtin/src/wait/status.rs (3)
56-58: The closure returned by
job_statusnow handles cases where the job is not found or is disowned, returningControlFlow::BreakwithExitStatus::NOT_FOUND. This change aligns with the PR objectives to treat disowned jobs as finished.75-77: The closure also removes the disowned job from the job set, which is a necessary step to prevent the job from being reported or affected by the
waitcommand in the future. This is a good practice for managing the job set's state.251-262: The test
status_of_disowned_jobis added to ensure that thejob_statusfunction correctly identifies and handles disowned jobs. This is an important test case that verifies the new functionality introduced in the PR.yash-builtin/src/jobs.rs (1)
- 73-75: The added documentation provides a warning about the non-portable behavior of the
jobsbuilt-in when used in a subshell. This is a valuable addition for users who may rely on the current behavior that is subject to change.yash-env/src/subshell.rs (3)
190-190: The call to
env.jobs.disown_all()ensures that jobs are disowned when a subshell is initiated, aligning with the PR objectives to handle jobs in subshells correctly.385-400: The test
jobs_disowned_in_subshellcorrectly checks the disowning behavior of jobs in a subshell. It verifies that a job added before starting the subshell is not owned within the subshell and is owned in the parent environment after the subshell exits.312-312: The import of
Jobis used in the testjobs_disowned_in_subshellto create a new job instance, which is necessary for the test.yash-env/src/job.rs (4)
93-98: The addition of the
is_ownedfield to theJobstruct is consistent with the PR's objective to handle job ownership, especially in subshells. This field will help in determining whether a job should be waited on by the current shell instance.115-115: The initialization of
is_ownedtotruein theJob::newmethod is appropriate as it assumes that a new job is owned by the shell that created it unless specified otherwise.591-597: The
disown_allmethod correctly iterates over all jobs and sets theiris_ownedflag tofalse. This method will be useful when creating subshells to ensure that the parent shell's jobs are not waited on by the subshell.848-860: The test
disowning_jobsverifies that thedisown_allmethod functions as expected by checking theis_ownedflag of jobs after disowning. This is a good practice to ensure that the new functionality works correctly.
The test script was copied from: https://github.com/magicant/yash/blob/8ed979daa54384b40728617cc52e78490cb72455/tests/wait-p.tst The test cases require a pseudo-terminal because they use the job control facilities. The pseudo-terminal handling code is based on: https://github.com/magicant/pseudo-terminal-wrapper
magicant
force-pushed
the
wait-builtin
branch
from
e7d321e
to
9980dd9
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- Cargo.lock
- yash/Cargo.toml
Files selected for processing (8)
- yash-builtin/src/jobs.rs (1 hunks)
- yash-builtin/src/wait.rs (1 hunks)
- yash-builtin/src/wait/status.rs (3 hunks)
- yash-env/src/job.rs (4 hunks)
- yash-env/src/subshell.rs (3 hunks)
- yash/tests/pty/mod.rs (1 hunks)
- yash/tests/scripted_test.rs (3 hunks)
- yash/tests/scripted_test/wait-p.sh (1 hunks)
Files skipped from review due to trivial changes (2)
- yash-builtin/src/wait.rs
- yash-builtin/src/wait/status.rs
Additional comments: 24
yash/tests/scripted_test/wait-p.sh (16)
5-8: The test case 'waiting for all jobs (+m)' seems to be testing the wait built-in without specifying any job ID, which should wait for all background jobs to complete. Ensure that the exit status of the background jobs is checked to confirm that
waitbehaves as expected.15-17: The test case 'waiting for specific single job (+m)' is testing the wait built-in with a specific job ID. It's important to verify that the exit status of the job is captured correctly by the
waitcommand.20-24: The test case 'waiting for specific many jobs (+m)' is testing the wait built-in with multiple job IDs. It should be verified that
waitreturns the exit status of the last job when multiple job IDs are provided.27-29: The test case 'waiting for unknown job (+m)' is testing the wait built-in with an invalid job ID. The expected behavior is that
waitshould return an exit status of 127 for unknown jobs. This needs to be verified.32-35: The test case 'jobs are not inherited to subshells (+m, -s)' is testing the isolation of job control between the parent shell and subshells. It's important to verify that jobs started in the parent shell are not waited on in the subshell.
38-39: The test case 'jobs are not inherited to subshells (+m, -c)' is similar to the previous one but uses the
-coption to run the shell command. The behavior should be consistent with the previous test case.42-45: The test case 'jobs are not propagated from subshells (+m)' is testing that jobs started in a subshell are not affecting the parent shell's job table. This is crucial for correct job control behavior and needs to be verified.
48-51: The test case 'waiting for all jobs (-m)' is testing the wait built-in without job control. It should be verified that
waitstill waits for all background jobs to complete even when job control is not enabled.58-60: The test case 'waiting for specific single job (-m)' is testing the wait built-in with a specific job ID without job control. It should be verified that
waitcorrectly waits for the specified job to complete.63-67: The test case 'waiting for specific many jobs (-m)' is testing the wait built-in with multiple job IDs without job control. It should be verified that
waitbehaves as expected, similar to the test case with job control enabled.70-72: The test case 'waiting for unknown job (-m)' is testing the wait built-in with an invalid job ID without job control. The expected behavior is that
waitshould return an exit status of 127 for unknown jobs. This needs to be verified.75-79: The test case 'specifying job ID' is testing the wait built-in with job control string IDs. It should be verified that
waitcorrectly identifies and waits for the jobs specified by their control strings.84-87: The test case 'jobs are not inherited to subshells (-m, -s)' is testing the isolation of job control between the parent shell and subshells without job control. It should be verified that jobs started in the parent shell are not waited on in the subshell.
90-91: The test case 'jobs are not inherited to subshells (+m, -c)' is similar to the previous one but uses the
-cmoption to run the shell command. The behavior should be consistent with the previous test case.94-97: The test case 'jobs are not propagated from subshells (-m)' is testing that jobs started in a subshell are not affecting the parent shell's job table without job control. This is crucial for correct job control behavior and needs to be verified.
101-116: The test case 'trap interrupts wait' is testing the behavior of
waitwhen an interrupt signal is received. It should be verified that the trap is set correctly and that the interrupt signal causeswaitto exit as expected.yash-builtin/src/jobs.rs (2)
73-75: The documentation has been updated to reflect the new behavior of the
jobsbuilt-in when used in a subshell, which now reports jobs from both the subshell and the parent shell. This change is significant as it affects the portability and expected behavior of the built-in in different environments.70-79: The code within the hunk is well-structured, follows Rust best practices, and includes comprehensive tests to ensure the functionality of the
jobsbuilt-in. The changes are consistent with the documented behavior and do not introduce any apparent issues.yash-env/src/subshell.rs (2)
190-190: The addition of
env.jobs.disown_all();ensures that all jobs are marked as disowned when a subshell is initiated. This aligns with the PR objectives to prevent jobs started in a parent shell from interfering with the subshell's environment.385-400: The test
jobs_disowned_in_subshellcorrectly checks the disowning behavior of jobs within a subshell. It verifies that a job added before starting the subshell is disowned inside the subshell and remains owned outside of it. This test supports the changes made to the job control logic and ensures that thedisown_allmethod works as expected.yash-env/src/job.rs (4)
93-98: The addition of the
is_ownedfield to theJobstruct is consistent with the PR's objective to handle jobs in subshells correctly. This field will help in identifying whether a job is owned by the current shell or inherited from a parent shell, which is important for the correct behavior of thewaitbuilt-in in subshells.115-115: The initialization of the
is_ownedfield totruein theJob::newmethod is appropriate, as it assumes that new jobs are owned by the shell that created them unless explicitly disowned.591-597: The
disown_allmethod correctly sets theis_ownedflag of all jobs tofalse. This method will be useful when creating subshells to ensure that the subshell does not wait for or report jobs that were started in the parent shell.848-860: The new test case
disowning_jobseffectively tests thedisown_allmethod by ensuring that all jobs in theJobSethave theiris_ownedflag set tofalseafter the method is called. This is a good practice to ensure that new features work as expected.
This pull request adds scripted tests for the wait built-in. It also fixes the bug where jobs started in the parent shell were affecting the behavior of the wait built-in when invoked in a subshell.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
waitbuilt-in command.Documentation