Rewrite utils and testing framework so code coverage works

[Arcterus]

The reason the code coverage seems so low is because it's not actually testing the utilities themselves at the moment as they are tested as subprocesses of the testing executable.

To fix this, I am thinking we need to pass stdin, stdout, and stderr to each util's uumain() and rewrite the utils to read and write from the given arguments rather than std::io::stdin() and such. The testing framework would then have to be modified to call uumain() with the given arguments rather than launching a subprocess with a modified stdin, for example. Unfortunately, doing so requires modifying every util, likely fairly invasively as we will also need to change anything that calls exit() to instead return a Result (to avoid crashing the testing executable). Additionally, any panics would just write to the normal stderr (e.g. any .unwrap()s).

[Arcterus]

A bonus of rewriting everything like this is that uutils could be embedded into programs (to be used as builtins for a shell, for example).

[ids1024]

This sounds like a good idea for several reasons.

I presume it would take Read and Write traits, with something like this:

pub fn uumain<R: Read, W1: Write, W2: Write>(args: Vec<String>,
                                             stdin: R, stdout: W1, stderr: W2) -> i32 {

And then pass io::stdin(), io::stdout(), and io::stderr() by default.

One minor issue with this is that it would not be possible to call .lock() on these object, since you just have Read/Write; this interferes with potential optimizations, which might be relevant for some things. Grepping through the code, .lock() is called in several places.

[ids1024]

This might be solved by just passing io::stdin().lock() etc. instead; I guess the locking isn't necessary unless multiple things are reading/writing in parallel? But that might cause issues I'm not thinking of...

[Arcterus]

Yep, that’s essentially what I’m thinking. I’m only really worried about passing io::stdin().lock() in the case where uutils is embedded in another program, as the parent program won’t be able to use stdin while the uutils code is running.

I’ll probably try to push some code outlining things later today or tomorrow.

[Arcterus]

I have some stuff here. I still need to work on ways to reduce boilerplate. Note that only arch and base32 have been modified to use the new interface.

[mssun]

I patched the util.rs file, then use kcov to calculate the code coverage. Note that this is just a temporary work around.

diff --git a/tests/common/util.rs b/tests/common/util.rs
index 1a0e420..fcfc680 100644
--- a/tests/common/util.rs
+++ b/tests/common/util.rs
@@ -453,7 +458,12 @@ impl UCommand {
             tmpd: None,
             has_run: false,
             raw: {
-                let mut cmd = Command::new(arg.as_ref());
+                let mut args = vec!["--verify", "--exclude-pattern=/.cargo", "--include-path=/uutils-coreutils-upstream/src/", "/uutils-coreutils-upstream/target/kcov"];
+                args.push(arg.as_ref().to_str().unwrap());
+                let mut cmd = Command::new("/.local/bin/kcov");
+                cmd.args(args);
                 cmd.current_dir(curdir.as_ref());
                 if env_clear {
                     if cfg!(windows) {

The overall line coverage is 59.3%. You can find details in attached reports.

Attached are the generated results:

• Coverage report - uutils.pdf
• kcov.tar.gz

[mssun]

Also, I suggest to remove the Codecov CI (badge, PR hook, and reports) for now or write a notice in README. Since the results are mis-leading. The number (20%) does not mean anything related to the code coverage and will not help PRs to improve their testcases.

Here is the outputs of cargo tarpaulin -v:

$ cargo tarpaulin -v
src/arch/arch.rs: 0/5
src/base32/base32.rs: 0/23
src/base64/base64.rs: 0/23
src/basename/basename.rs: 0/36
src/cat/cat.rs: 0/174
src/chgrp/chgrp.rs: 0/153
src/chmod/chmod.rs: 0/106
src/chown/chown.rs: 0/207
src/chroot/chroot.rs: 0/83
src/cksum/build.rs: 0/1
src/cksum/cksum.rs: 0/45

...

tests/test_touch.rs: 182/183
tests/test_tr.rs: 39/52
tests/test_true.rs: 3/3
tests/test_truncate.rs: 15/15
tests/test_tsort.rs: 3/4
tests/test_unexpand.rs: 42/56
tests/test_uniq.rs: 56/70
tests/test_unlink.rs: 25/28
tests/test_wc.rs: 21/28
tests/test_who.rs: 34/35
tests/tests.rs: 1/1

22.60% coverage, 3988/17647 lines covered
Tarpaulin finished

The tarpaulin result shows that only sources under tests are covered by tests. I believe this result is not helpful for developers as well as users.

[Arcterus]

Rather than remove the Codecov CI, I think it would make more sense to switch to kcov temporarily.

[mssun]

breakpoint-based code coverage (like kcov) are very slow. It took me hours to get the report. rustls's author also mentioned this in his attempt.

Prior to this work, I used kcov. This produced good output, but each test process took around three seconds to start. One of the test suites runs around 1000 processes, so this meant a full test run took almost an hour (compared to 50 seconds without coverage).

https://jbp.io/2017/07/19/measuring-test-coverage-of-rust-programs.html

I'm not sure if kcov can be integrated in the CI.

[Arcterus]

Hm, well, in that case I guess dropping code coverage for the time being is the best solution.

[mssun]

Thanks. In the meantime, I also create an issue to the tarpaulin project to support assert_cli. https://github.com/xd009642/tarpaulin/issues/105

[sylvestre]

@rivy did it See: https://codecov.io/gh/uutils/coreutils/

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.