coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

chmod: fix incorrect test

Open BenWiederhake opened this issue 1 year ago • 4 comments

First of all, let's observe that uutils and GNU coreutils behave identically in a special edgecase, and emit exactly the same warning message:

$ touch file
$ chmod =0777 file; ls -l file
-rwxrwxrwx 1 user user 0 Aug 12 00:02 file
$ chmod -w file # GNU says "r-xr-xrwx" in the error message
chmod: file: new permissions are r-xr-xrwx, not r-xr-xr-x
[$? = 1]
$ chmod =0777 file; ls -l file # (reset test setup)
-rwxrwxrwx 1 user user 0 Aug 12 00:02 file
$ cargo run -q --features chmod chmod -w file # uutils correctly says "r-xr-xrwx" in the error message, too
chmod: file: new permissions are r-xr-xrwx, not r-xr-xr-x
[$? = 1]

So uutils is already correct.

However, this test somehow fails:

$ cargo test -q --features chmod -- test_chmod_ugoa
<SNIP irrelevant output>
Diff < left / right > :
<chmod: file: new permissions are r-xr-xrwx, not r-xr-xr-x
>chmod: file: new permissions are r-xrwxrwx, not r-xr-xr-x
<SNIP irrelevant output>

In other words, the test code itself is wrong, because it asserts the wrong output. Also, for the record, even uutils changes the permissions to -r-xr-xrwx, which is correct.

This PR fixes the incorrect test.

Note that this does NOT fix #6637, which is a bug in the way that cargo test is invoked.

BenWiederhake avatar Aug 11 '24 22:08 BenWiederhake

GNU testsuite comparison:

Congrats! The gnu test tests/timeout/timeout is no longer failing!

github-actions[bot] avatar Aug 11 '24 22:08 github-actions[bot]

Looks like we depend on the current umask or something like that.

EDIT: No, I don't get it. Why would chmod fail to remove the gw bit only on some platforms?! I don't understand.

BenWiederhake avatar Aug 11 '24 23:08 BenWiederhake

From the chmod(2) man page:

If none of these are given, the effect is as if (a) were given, but bits that are set in the umask are not affected.

So on one machine (linux):

➜  coreutils git:(main) ✗ umask
022

Then on another linux machine (different distro):

rseymour@onassis:~$ umask
0022

All of which is compatible with -w only changing the first 7 to a 5, ie rwxrwxrwx to r-xrwxrwx. So the test has to check (or set and reset?) the umask

rseymour avatar Aug 12 '24 02:08 rseymour

2 more small discoveries. The test_cp unit test already calls in procfs, so that shouldn't be a problem to find the umask during the test. And the following umask change creates a panic in the test:

$ umask 222
$ cargo test -q --features chmod -- test_chmod_ugoa
[...]
running 1 test
test_chmod::test_chmod_ugoa --- FAILED

failures:

---- test_chmod::test_chmod_ugoa stdout ----
thread 'test_chmod::test_chmod_ugoa' panicked at tests/by-util/test_chmod.rs:27:10:
called `Result::unwrap()` on an `Err` value: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

rseymour avatar Aug 12 '24 22:08 rseymour

I don't understand this topic enough to maintain this PR.

@rseymour, if you want to take over, feel free to take any/all/none of this code. :)

BenWiederhake avatar Sep 07 '24 19:09 BenWiederhake