`tests/util`: Implement enhancements of `CmdResult` #4259 #4261

Joining7943 · 2023-01-04T09:11:09Z

This pr enhances CmdResult. The following changes are introduced in CmdResult:

Add methods stderr_str_apply, stderr_apply, stdout_str_apply, stdout_apply to apply a function to the stderr or stdout returning a new CmdResult on which the assertions in CmdResult can be run.
Add methods stderr_str_check, stderr_check, stdout_str_check, stdout_check. Enables running arbitrary functions returing a bool to verify the stdout or stderr in addition to the assertions CmdResult already provides.
Remove trimming of stderr in stderr_is. Add a separate method stderr_trimmed_is for asserting stderr trimmed of whitespace.
Remove the code and success fields and instead store the ExitStatus of the child process in the exit_status field. A
Add methods like signal_is, signal_name_is to assert the received signal on unix systems.
Improve the error messages of stderr_is_bytes and stdout_is_bytes to include (in addition to the diff of the bytes arrays) the stdout, stderr converted from bytes to a string if possible. If not possible the UTF-8 error message is shown instead of the string.

The integration tests needed to be adjusted to the new situation in stderr_is and now assert whitespace which was previously trimmed.

Some tests in test_tr fail now and needed to be disabled until fixed. tr produces sometimes two newlines at the end of the output instead of just one. I cross checked some those tests randomly with gnu's tr, to ensure that only one newline is expected.

Closes #4259

github-actions · 2023-01-05T16:54:30Z

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-01-05T17:24:33Z

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-01-06T10:51:16Z

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

tertsdiepraam · 2023-01-06T12:01:55Z

Instead of applying str::trim I think I prefer checking against untrimmed strings in most of these cases. So

cmdresult.stdout_is("cat: some error");
// becomes
cmdresult.stdout_is("cat: some error\n");
// instead of
cmdresult
    .stdout_trimmed_is("cat: some error");
// or
cmdresult
    .stdout_str_apply(str::trim)
    .stdout_is("cat: some error");

because that's a more precise test anyway.

Joining7943 · 2023-01-06T13:01:44Z

Instead of applying str::trim I think I prefer checking against untrimmed strings in most of these cases.

I'd actually prefer, too. I'll try to change as much of these usages.

Joining7943 · 2023-01-06T13:27:03Z

What do you think of TestScenario::scmd? I don't needed it often but if so, it's nice to have it and spares the tedious setup of a different shell command for windows and unix.

tertsdiepraam · 2023-01-06T13:38:43Z

The functionality is good, but I'd prefer if it was a separate setting on UCommand. I find cmd, ccmd & ucmd confusing enough already 😄

Joining7943 · 2023-01-06T15:21:07Z

The functionality is good, but I'd prefer if it was a separate setting on UCommand. I find cmd, ccmd & ucmd confusing enough already

In UCommand it needs to be a new_something -> Self method and can't be a setting or else it breaks the UCommand::arg, UCommand::args methods. However, I moved some of the functionality into UCommand, so there's more than one way to construct a shell command. If you have an idea how to do it differently, I'd love to hear. I've changed the name from TestScenario::scmd to TestScenario::shell_cmd. I hope this is less confusing, just thought scmd enrows into the naming scheme in TestScenario.

tertsdiepraam · 2023-01-06T15:34:13Z

It would work if we only construct the raw Command when the command gets run and keep track of the executable, arguments and env vars ourselves until that point. That makes it complicated enough for a separate PR though.

github-actions · 2023-01-06T16:09:03Z

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

Joining7943 · 2023-01-06T16:20:11Z

Ok, that would work, but is it worth it? This solution is less involved and provides pretty much the same functionality. Or is there any other gain in tracking the args, env vars etc.?

github-actions · 2023-01-06T16:23:47Z

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

tertsdiepraam · 2023-01-06T16:40:24Z

We could use it to provide better error messages maybe? I'm not sure either.

Joining7943 · 2023-01-06T17:02:24Z

We could use it to provide better error messages maybe? I'm not sure either.

Me neither. But, I actually like your idea, also because this is the way how a builder like UCommand usually works, so refactoring UCommand might be a good idea.

However, would you be fine with keeping TestScenario::shell_cmd and UCommand::new_shell_cmd the way it is for now? I can create a UCommand refactoring issue and then we can track these two methods, so we don't forget about it.

Joining7943 · 2023-01-06T18:54:43Z

This pr would be ready so far. Please tell me if you're ok with keeping the TestScenario::shell_cmd and UCommand::new_shell_cmd methods.

Please note that I updated the pr description. Some tests in test_tr needed to be disabled because of 2 newlines instead of an expected amount of 1, so there's an issue in tr somewhere.

github-actions · 2023-01-06T19:12:22Z

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

tertsdiepraam · 2023-01-07T12:07:48Z

Please tell me if you're ok with keeping the TestScenario::shell_cmd and UCommand::new_shell_cmd methods.

Do you think they're useful on their own? If were gonna do the same thing a different way soon and remove these I don't think we need to include them here.

tertsdiepraam · 2023-01-07T12:10:15Z

tests/common/util.rs

+    /// Return the exit status of the child process, if any.
+    ///
+    /// Returns None if the child process is still running or hasn't been started.
+    pub fn maybe_exit_status(&self) -> Option<ExitStatus> {


Nit: I think try_exit_status is idiomatic Rust

dunno, just tried something here. I searched the stdlib once and found no clear answer on how a naming scheme should look like for methods that return an Option. Try is definitely a good choice for methods that return a Result. Most of the time methods that return an Option are not given a special prefix, seldomly they are prefixed with try or even maybe. Sometimes they prefix variables that contain an option with maybe. If you'd agree, in our case it's probably best to remove the maybe prefix, since this is just a getter and we don't really try to get something.

I overlooked, that I named another method exit_status already. It's try_exit_status, then, if that's preferred over maybe_exit_status.

tests/common/util.rs

tertsdiepraam · 2023-01-07T12:16:28Z

tests/common/util.rs

+    pub fn stderr_trimmed_is<T: AsRef<str>>(&self, msg: T) -> &Self {
+        assert_eq!(self.stderr_str().trim(), msg.as_ref());
+        self
+    }


Do we still need this now that we check the newlines explicitly? I think the problem you found with tr show that checking the newlines explicitly is the way to go.

Do we still need this now that we check the newlines explicitly?

Nope. I'll remove the method.

Joining7943 · 2023-01-07T15:25:14Z

Do you think they're useful on their own? If were gonna do the same thing a different way soon and remove these I don't think we need to include them here.

ok, let's remove them and address this in another pr.

github-actions · 2023-01-07T18:47:15Z

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

Joining7943 · 2023-01-11T11:37:52Z

Everything alright with this pr? I addressed all your comments. If it's ok I don't squash the commits any further.

tertsdiepraam

Yeah looks good! Just one more nit. As a final squash maybe you could add the signals feature to the commit that introduced the need for them.

tertsdiepraam · 2023-01-11T11:45:55Z

tests/common/util.rs

-    /// zero-exit from running the Command?
-    /// see [`success`]
-    success: bool,
+    // /// exit status for command (if there is one)


There's a double comment here

Joining7943 · 2023-01-11T13:26:05Z

As a final squash maybe you could add the signals feature to the commit that introduced the need for them.

Sure, no problem.

github-actions · 2023-01-11T13:39:28Z

GNU testsuite comparison:

GNU test failed: tests/rm/rm2. tests/rm/rm2 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

Joining7943 · 2023-01-19T15:55:14Z

@tertsdiepraam my account was suspended since last week. I'm not sure about the reason for the suspend, but github unsuspended my account today after I wrote to the github support several times. Seems, that all prs, issues etc. which I created, disappeared for that period. However, it looks like noting's lost and I rebased onto main and resolved all conflicts with the main branch. Besides that, I made no changes and we're back to the state of last week.

…tdout, stderr before asserting

…rite tests for CmdResult

… unix systems Add tests for these signal methods. Include `signals` feature in Cargo.toml uucore dev-dependencies.

…n arbitrary assertions

…derr_trimmed_is. Fix tests assert whitespace instead of trimming it. Disable some tests in `test_tr` because `tr` produces too many newlines.

…`, `stdout_is_bytes`

github-actions · 2023-01-22T15:00:30Z

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

tertsdiepraam · 2023-01-25T09:06:27Z

my account was suspended since last week

Glad to see you're back!

Joining7943 · 2023-01-25T17:43:44Z

Glad to see you're back!

yeah me too! Honestly missed that working on uutils.

Joining7943 mentioned this pull request Jan 4, 2023

tests/util: Enhance CmdResult #4259

Closed

Joining7943 force-pushed the tests-util-enhance-cmdresult branch from 410c896 to d269c62 Compare January 5, 2023 16:35

Joining7943 force-pushed the tests-util-enhance-cmdresult branch from fc77e49 to b2e4a22 Compare January 6, 2023 09:50

Joining7943 force-pushed the tests-util-enhance-cmdresult branch from d3756ea to 4d311be Compare January 6, 2023 15:23

Joining7943 force-pushed the tests-util-enhance-cmdresult branch from 036c75f to 6a54f3c Compare January 6, 2023 18:23

Joining7943 marked this pull request as ready for review January 6, 2023 18:50

tertsdiepraam reviewed Jan 7, 2023

View reviewed changes

Joining7943 force-pushed the tests-util-enhance-cmdresult branch 2 times, most recently from 461003f to db6e429 Compare January 7, 2023 16:54

tertsdiepraam reviewed Jan 11, 2023

View reviewed changes

Joining7943 force-pushed the tests-util-enhance-cmdresult branch from 548df96 to eee6cdc Compare January 11, 2023 12:32

Joining7943 force-pushed the tests-util-enhance-cmdresult branch from eee6cdc to 338e951 Compare January 19, 2023 14:39

Joining7943 added 6 commits January 22, 2023 14:48

tests/util: Introduce CmdResult::stdout_str_apply ... to preprocess s…

37e06ed

…tdout, stderr before asserting

tests/util: Use ExitStatus instead of code, success in CmdResult. Rew…

19db042

…rite tests for CmdResult

tests/util: Add methods in CmdResult to assert the received signal on…

c2ad54f

… unix systems Add tests for these signal methods. Include `signals` feature in Cargo.toml uucore dev-dependencies.

tests/util: Add stdout_check, stdout_str_check ... to CmdResult to ru…

7c0ed03

…n arbitrary assertions

tests/util: Do not trim stderr in CmdResult::stderr_is. Add method st…

1fadeb4

…derr_trimmed_is. Fix tests assert whitespace instead of trimming it. Disable some tests in `test_tr` because `tr` produces too many newlines.

tests/util: Improve assertion messages of `CmdResult::stderr_is_bytes…

78a3810

…`, `stdout_is_bytes`

Joining7943 force-pushed the tests-util-enhance-cmdresult branch from 338e951 to 78a3810 Compare January 22, 2023 13:56

This was referenced Jan 22, 2023

tests/util: Small Refactor/Fixes of UCommand and add method to run a UCommand in a shell platform independently #4293

Merged

tr: Error output ends with two newlines instead of one #4301

Closed

Joining7943 requested a review from tertsdiepraam January 24, 2023 19:10

tertsdiepraam approved these changes Jan 25, 2023

View reviewed changes

tertsdiepraam merged commit fc7e51a into uutils:main Jan 25, 2023

Joining7943 deleted the tests-util-enhance-cmdresult branch January 25, 2023 17:44

Joining7943 mentioned this pull request Jan 27, 2023

tests/util: Don't trim output in CmdResult::stdout_matches and stdout_does_not_match #4304

Merged

Uh oh!

tests/util: Implement enhancements of CmdResult #4259 #4261

tests/util: Implement enhancements of CmdResult #4259 #4261

Uh oh!

Conversation

Joining7943 commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 5, 2023

Uh oh!

github-actions bot commented Jan 5, 2023

Uh oh!

github-actions bot commented Jan 6, 2023

Uh oh!

tertsdiepraam commented Jan 6, 2023

Uh oh!

Joining7943 commented Jan 6, 2023

Uh oh!

Joining7943 commented Jan 6, 2023

Uh oh!

tertsdiepraam commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Joining7943 commented Jan 6, 2023

Uh oh!

tertsdiepraam commented Jan 6, 2023

Uh oh!

github-actions bot commented Jan 6, 2023

Uh oh!

Joining7943 commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 6, 2023

Uh oh!

tertsdiepraam commented Jan 6, 2023

Uh oh!

Joining7943 commented Jan 6, 2023

Uh oh!

Joining7943 commented Jan 6, 2023

Uh oh!

github-actions bot commented Jan 6, 2023

Uh oh!

tertsdiepraam commented Jan 7, 2023

Uh oh!

tertsdiepraam Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

Joining7943 Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

Joining7943 Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tertsdiepraam Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

Joining7943 Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joining7943 commented Jan 7, 2023

Uh oh!

github-actions bot commented Jan 7, 2023

Uh oh!

Joining7943 commented Jan 11, 2023

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

Joining7943 Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

Joining7943 commented Jan 11, 2023

Uh oh!

github-actions bot commented Jan 11, 2023

Uh oh!

`tests/util`: Implement enhancements of `CmdResult` #4259 #4261

`tests/util`: Implement enhancements of `CmdResult` #4259 #4261

Joining7943 commented Jan 4, 2023 •

edited

Loading

tertsdiepraam commented Jan 6, 2023 •

edited

Loading

Joining7943 commented Jan 6, 2023 •

edited

Loading

Joining7943 Jan 7, 2023 •

edited

Loading