Skip to content

Conversation

@Gilnaa
Copy link
Contributor

@Gilnaa Gilnaa commented Apr 2, 2021

In order to ease the reduction of CmdResult fields directly,
I've added the following measures:

  • CmdResult::{success,failure} and UCommand::{failed,succeeded} now
    print the stdout/stderr. This is done to reduce prints in tests. (as suggested here)
  • Added code_is that asserts that return code
  • Fixed documentation of stdout_only which stated that it trims the string.
  • Added some more utilities for actions that previously required accessing stdout or stdout_str().
  • Fix documentation of stdout_only which claimed it was trimming the output.
  • (some) Panicking utilities now has track_caller slapped on them to point accurately at the call location in the tests.

On different PRs:

There's probably a use for stdout_trimmed(), since it's used in many places.
After this PR, grepping the repo for the regex \.stdout[^_\( ] (with match-whole-word) returns only 156 in 18 files.

@Gilnaa Gilnaa force-pushed the master branch 2 times, most recently from a538ae4 to f8f1c5a Compare April 2, 2021 21:05
@Gilnaa
Copy link
Contributor Author

Gilnaa commented Apr 2, 2021

Ummm, just realized that track_caller is only available from Rust 1.46 and and the MSRV is 1.40.

I can either remove it or, or add it conditionally with a cfg_attr, like done here, for example.
This will require an extra build time dep on something, like rustc_version or autocfg.
Will do this happily if a maintainer agrees; didn't want to add another dependency without approval.

@Gilnaa Gilnaa force-pushed the master branch 6 times, most recently from c31522e to 5f3fcce Compare April 3, 2021 16:30
@jhscheer
Copy link
Contributor

jhscheer commented Apr 5, 2021

I like these changes in combination with #1982, however you should probably split this PR into multiple PRs.

@Gilnaa
Copy link
Contributor Author

Gilnaa commented Apr 5, 2021

Sure, no problem.
According to any specific criteria, or just to make it a bit more digestible?

@jhscheer
Copy link
Contributor

jhscheer commented Apr 5, 2021

Yes more digestible is good :)
Based on your bullet points and what belongs together with regards to content, I'd say:
One PR for everything that belongs to the testing framework (first 5 bullet points). And then one PR for each of the last 3 bullet points.

@Gilnaa
Copy link
Contributor Author

Gilnaa commented Apr 5, 2021

Sure thing.
Will leave this PR with the start of the and split off the rest

@Gilnaa Gilnaa force-pushed the master branch 2 times, most recently from 2d7b1dd to 3fcc216 Compare April 5, 2021 19:55
@Gilnaa Gilnaa changed the title Remove some usage of CmdResult fields in tests Add some test utils needed for major test refactoring Apr 5, 2021
@Gilnaa
Copy link
Contributor Author

Gilnaa commented Apr 5, 2021

Split off macro changes into #2033
Most of the tests changes are in #2034
(I left of the ls changes since that file is huge and incomplete)

@sylvestre
Copy link
Contributor

I love this change, many thanks :)

it is possible to write some unit tests?

@Gilnaa
Copy link
Contributor Author

Gilnaa commented Apr 9, 2021

Sure. Is there a test suite for the test utils?

@sylvestre
Copy link
Contributor

@Gilnaa
Copy link
Contributor Author

Gilnaa commented Apr 9, 2021

I meant to ask if it's a standard practice to test the test utils themselves.

I'll add some

@Gilnaa Gilnaa force-pushed the master branch 2 times, most recently from bf3b37a to c5d41a9 Compare April 10, 2021 18:35
@sylvestre
Copy link
Contributor

could you please fix the conflict? thanks

@Gilnaa
Copy link
Contributor Author

Gilnaa commented Apr 10, 2021

@sylvestre Rebased over master

@sylvestre sylvestre merged commit c224b95 into uutils:master Apr 11, 2021
@jhscheer jhscheer mentioned this pull request Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants