-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests/common/util
: Implement UChild, an abstraction for std::process::Child, to return UChild from run_no_wait instead
#4136
Conversation
GNU testsuite comparison:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Could you add or refactor some tests to show why it's better than the normal test library? If you're adding a new test, please also show an example of what it would need to look like without the TestRunner
.
Some first observations:
- The commands should probably still be a
UCommand
that you pass to theTestRunner
so that we don't need to specify each possible option in theTestRunner
struct, provide builder methods for them, etc.. The way you've written it now feels error prone, because we need to keep the implementation ofTestRunner
in sync with the implementation oftail
itself. - I'd like a stronger distinction between a command and a command result, like what
UCommand
/CmdResult
has. Functions likeassert_stdout_matches
should not be implemented on the runner. - In general, I would advise you to not make this
TestRunner
do too much. It's doing argument handling, setting up the fixtures, running and managing a process, and checking the output. That's a big burden to put on one struct. It seems to me that your focus should be in managing the child process, because we have acceptable abstractions in place for the other jobs (UCommand
,AtPath
,CmdResult
). To take it to an extreme, maybe it's possible to include most of the functionality as just a wrapper aroundstd::process::Child
? All the stuff like getting the intermediatestdout
,kill_asserted
,assert_alive
should fit in there. Other things, likestderr_to_stdout
could actually be interesting for other utils, too, so that could go intoUCommand
. - It's unclear to me what
run_cmd
is for. Note that you can already callsh
(or other commands) quite easily withtest_scenario.ccmd("sh")
and you get all theUCommand
functionality. - I don't like how it uses things from
tail
. TheFollowMode
andSignum
could just be supplied as strings, instead.tail::text
seems to be unused altogether, not just on Windows.
Thanks for your comments :) Perhaps something I'm not sure if it's clear, the TestRunner never intends to or will replace UCommand, CmdResult etc. and it is delegating to them where it can. Sure, if you like to you can do all the stuff as usual with the TestRunner, although if you don't need stderr_to_stdout or handling and asserting child processes you're maybe better off with the procedure as usual. The TestRunner provides stderr_to_stdout (also for the usual run), run_no_wait, run_cmd and the assertions around stderr_to_stdout and child processes, together with some convenience around setting up the arguments for the actual run. I hope this gives you a better idea of what the TestRunner provides and maybe answers some of your points. If the stderr_to_stdout functionality is useful for others and goes into UCommand I'm happy to be able to remove or at least drastically reduce the handling around it in the
I'm not sure which parts are error prone and need to be in sync with tail (maybe besides the two arguments you mentioned, although I think it's more like a feature than a bug).
That's something very useful that can be used in run_cmd.
I like that.
Yep, that's something I've overseen, text is used in the tests later (which currently aren't there) and then the windows comment fits again ;) |
Yeah this was clear!
Yeah I can think of some cases where this might be useful. We've had trouble testing the interleaving of stdout and stderr for
I was thinking about the arguments, specifically. For example, if I add a new argument and want to add it to the
That's a lot of things to do if the alternative is just using a slice of strings just like it's used on the command line. I could easily see people forgetting to add it to |
Here's the basics for stderr_to_stdout in UCommand. I have tested it by trying a little bit around and it worked so far. I'm going to write some proper tests. Maybe you can give me a short feedback upfront? |
GNU testsuite comparison:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is (as always) more difficult than I imagined :) I was only thinking about the run
case, which is indeed easy, but run_no_wait
is difficult. Maybe we can make this easier by changing the API.
Maybe something like this:
let file = tempfile::NamedTempFile::new().unwrap();
scene.ucmd().stdout_and_stderr(file.as_file()).run();
Now, the ownership stays of the file with the caller, but stdout and stderr still get interleaved.
Alternatively, we are already thinking about adding some abstraction over std::process::Child
, so this could be the first step:
pub struct UChild {
raw: std::process::Child,
output: Option<tempfile::NamedFile>,
}
There would be only one run_no_wait
method, which always returns a UChild
. And then we can implement methods like output
, kill
, assert_alive
, etc. on this struct. I do have to note that this would be a big change, because we'd need to refactor everything that uses run_no_wait
(used 53 times).
But there's a big upside to this, namely that it allows us to do something we can't do now. We can't currently do UCommand -> Child -> CmdResult
, but only UCommand -> CmdResult
(via run
). If we make UChild::wait
return a CmdResult
then we can do UCommand -> UChild -> CmdResult
. And that also means that these two are identical, which is cool:
command.run();
command.run_no_wait().wait();
What do you think?
I definitely like the abstraction :) Sounds really good. It's a big task, but on the long run I think too the outcome would be worth it. To be honest I'm not really convinced of the first one. You have this cumbersome setup in the test cases and have to care about the tempfile where you actually only want the output. I'm not a big fan of this extra method, too (maybe there's also a nicer name), but the CapturedOutput can return this tempfile too, if this is something you need. Alternatively passing something like CapturedOutput into stderr_to_stderr would be something I could live with. |
Yeah that's what I'm thinking too. 53 uses is is also not too bad and 34 of those uses are in
Something like this? (I'm renaming it to let out = CombinedOutput::new();
scene.ucmd().stdout_and_stderr(&out).run(); I quite like this! As a sidenote, I want to thank you for cooperating on splitting the other PR up. I think this now enables us to have super interesting discussions and I'm excited to see where all the ideas you have go! |
6452d95
to
ffb8ec6
Compare
I like it, too. I'm looking forward to the discussions. So, what's the plan, regarding the refactor and |
Up to you! If you want to do the |
The name is perfect. I'll figure something out, then. |
GNU testsuite comparison:
|
ffb8ec6
to
0550ea3
Compare
Here's a first implementation of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Liking this a lot already. The tests looks so clean! I have some suggestions about the API below.
I haven't started with stdout_and_stderr(CombinedOutput). I'm not exactly sure if we actually need it.
Yeah I don't think we need stdout_and_stderr(CombinedOutput)
with this.
GNU testsuite comparison:
|
GNU testsuite comparison:
|
ab28041
to
b7a60a3
Compare
I've refactored all tests and I've changed piping input to the child process' stdin happen in a separate thread (in The changed piping changes the test situation a bit (I think to the better) and there are failures in
Are you familiar with |
I can look more into it later, but have you tried with GNU tee and tac instead and do you get the same errors there? |
This would would be great!
No. How can I run the cargo tests with gnu? |
Only manually by changing |
Ah ok... I ran the failing test_tee tests:
with gnu's tail and they fail, too. The tac tests fail only on windows pretty randomly. Can't test with original gnu utils there, I think. |
Here's the test output:
|
My current state of investigation is: Regarding the tac tests, I would like to disable them on windows. |
tests/tail
: Test runner implementationtests/common/util
: Implement UChild, an abstraction for std::process::Child, to return UChild from run_no_wait instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love where this is headed. Most of my comments are focused on removing or hiding parts of the API, I think we should make this as simple as possible for contributors to understand (and the test framework has already a very big API).
I'm also gonna say this now, before we move further. You've got a lot of commits here, but the changes are too big that I don't want to squash them all into one. So it'd be great if you could do some cleanup yourself there.
As for tac
, maybe pipe_in_and_wait
is wrong and it's some concurrency bug? Maybe it exists without the thread having written anything somehow.
…e tests on windows. Summary: * Disable test_retry6 on android because of intermittent failures * Use wait() instead of wait_with_output in test_cat, test_cp, test_sort * tests/sort: Simplify usage of test_sigpipe_panic * Fix tests in test_tee. tests/tac: There was a change in the `tests/common/util.rs` test api concerning piped input which may have revealed a bug in the implementation of tac. Please see also uutils#4136
d171739
to
60af914
Compare
Almost, it all looks great and it's just the clippy things now! |
GNU testsuite comparison:
|
…e tests on windows. Summary: * Disable test_retry6 on android because of intermittent failures * Use wait() instead of wait_with_output in test_cat, test_cp, test_sort * tests/sort: Simplify usage of test_sigpipe_panic * Fix tests in test_tee. tests/tac: There was a change in the `tests/common/util.rs` test api concerning piped input which may have revealed a bug in the implementation of tac. Please see also uutils#4136
60af914
to
72fa832
Compare
GNU testsuite comparison:
|
…apture output as default See pr uutils#4136 (uutils#4136)
…e tests on windows. Summary: * Disable test_retry6 on android because of intermittent failures * Use wait() instead of wait_with_output in test_cat, test_cp, test_sort * tests/sort: Simplify usage of test_sigpipe_panic * Fix tests in test_tee. tests/tac: There was a change in the `tests/common/util.rs` test api concerning piped input which may have revealed a bug in the implementation of tac. Please see also uutils#4136
3024ad2
to
f7197b4
Compare
GNU testsuite comparison:
|
GNU testsuite comparison:
|
…apture output as default See pr uutils#4136 (uutils#4136)
…nd return UChild tests/tail: * test_stdin_redirect_file:. Test fails now when assert_alive()! The follow test `tail -f < file` where file's content is `foo` fails with: Assertion failed. Expected 'tail' to be running but exited with status=exit status: 0 I also tried on the command line and can confirm that tail isn't runnning when following by descriptor. The test is deactivated until the implementation is fixed. * test_follow_stdin_descriptor * test_follow_stdin_explicit_indefinitely. * test_follow_single * test_follow_non_utf8_bytes * test_follow_multiple * test_follow_name_multiple * test_follow_invalid_pid * test_single_big_args * test_retry3 * test_retry4 * test_retry5 * test_retry7 * test_retry8 * test_retry9 * test_follow_descriptor_vs_rename1 * test_follow_descriptor_vs_rename2 * test_follow_name_retry_headers * test_follow_name_remove * test_follow_name_truncate1 * test_follow_name_truncate2 * test_follow_name_truncate3 * test_follow_name_truncate4 * test_follow_truncate_fast * test_follow_name_move_create1 * test_follow_name_move_create2 * test_follow_name_move1 * test_follow_name_move2 * test_follow_name_move_retry1 * test_follow_name_move_retry2 * test_follow_inotify_only_regular * test_fifo * test_illegal_seek tests/cat: * test_dev_full * test_dev_full_show_all * test_dev_random * test_fifo_symlink tests/dd: * test_random_73k_test_lazy_fullblock * test_sync_delayed_reader tests/factor: * test_parallel tests/rm: * test_rm_force_prompts_order * test_rm_descend_directory * test_rm_prompts tests/seq: * the helper run method tests/sort: * test_sigpipe_panic tests/tee: * the helper run_tee method tests/tty: * test_tty module tests/yes: * the helper run method
A short summary of changes: * Add some basic tests for UChild and the run methods. * Try more often in a fixed interval to create the tempfile for CapturedOutput. * Fix drop order of struct fields for better cleanup of temporary files/dirs. * Mark UChild::wait_with_output and UChild::pipe_in_and_wait_with_output deprecated * Make CapturedOutput private * Panic in stdout_all, stdout_all_bytes etc. if output is not captured. * Rename some methods, refactor, clean up, fix documentation, add try_... methods
…e tests on windows. Summary: * Disable test_retry6 on android because of intermittent failures * Use wait() instead of wait_with_output in test_cat, test_cp, test_sort * tests/sort: Simplify usage of test_sigpipe_panic * Fix tests in test_tee. tests/tac: There was a change in the `tests/common/util.rs` test api concerning piped input which may have revealed a bug in the implementation of tac. Please see also uutils#4136
f7197b4
to
4f599dc
Compare
…e tests on windows. Summary: * Disable test_retry6 on android because of intermittent failures * Use wait() instead of wait_with_output in test_cat, test_cp, test_sort * tests/sort: Simplify usage of test_sigpipe_panic * Fix tests in test_tee. tests/tac: There was a change in the `tests/common/util.rs` test api concerning piped input which may have revealed a bug in the implementation of tac. Please see also uutils#4136
4f599dc
to
4a2ced5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I'm super happy with how this turned out! Thanks!
Sure. I'm glad to hear. |
GNU testsuite comparison:
|
looks great indeed, thanks :) |
UChild provides:
Changes in UCommand:
Stdio::piped
for stdout and stderr.stderr_to_stdout
to redirect stderr to stdoutNote, that some tests needed to be adjusted to the new return value of
UCommand::run_no_wait
.