id: revamp to pass more of GNU's Testsuite

[jhscheer]

Initially this was just a little PR to close #2351, but @sylvestre 's comment that the zero.sh test from GNU's Testsuite was failing led me go down a rabbit hole which resulted in discovering and fixing multiple underlying bugs first before the Testsuite was satisfied.

TLDR:
Together with the bug fixes from #2371 and #2416 this PR is a revamp of uu_id.
This revamp passes the relevant tests from GNU's Testsuite that currently fail on master branch.

============================================================================                                                                       
Testsuite summary for GNU coreutils 8.32                                                                                                           
============================================================================
PASS: tests/id/uid.sh
PASS: tests/id/zero.sh

Long version:
uu_id was originally based on BSD's id (noticeable in functionality, usage text, options text, etc.) and synced with:

 http://ftp-archive.freebsd.org/mirror/FreeBSD-Archive/old-releases/i386/1.0-RELEASE/ports/shellutils/src/id.c
 http://www.opensource.apple.com/source/shell_cmds/shell_cmds-118/id/id.c

In this PR:

• uu_id was partially rewritten in order for stdout/stderr/exit_code
  to be conform with GNU coreutils (8.32) testsuite for id.

• uu_id passes GNU's coreutils Testsuite (8.32)
  for "tests/id/uid.sh" and "tests/id/zero/sh".

• uu_id now supports multiple users (a feature that was introduced in coreutils 8.31)

• uu_id now supports '--zero', this flag does not exist for BSD's id,
  therefore '--zero' is only allowed together with other options that are available on GNU's id.

• Help text is now based on BSD's id manpage and GNU's id manpage.

• A bunch of requires/conflicts rules for clap options were added.

During debugging I made heavily use of calling the GNU coreutils (g)id binary in $PATH in order to gather reference values.
I came up with a little frame work (related: #2391) to help skip tests in environments where the necessary id binary doesn't include a coreutils version string, or the version is too low.
This framework could be useful for other uutils where the reference values are not known beforehand.
Or it could be integrated in common/util::TestScenario so that test case authors could just run ucmd() and test results would automatically show if stdout/stderr/exit_code differ from the respective GNU coreutils binary output.

Also thanks @tertsdiepraam for all the great help, feedback and discussions!

[sylvestre]

Great stuff. I retriggered the CI to get coverage results.

GNU is failing on:

2021-06-06T01:59:23.5201151Z FAIL: tests/id/zero
2021-06-06T01:59:23.5201827Z ===================
2021-06-06T01:59:23.5202035Z 
2021-06-06T01:59:23.5202821Z uid=1001(runner) gid=121(docker) groups=121(docker),4(adm),101(systemd-journal)
2021-06-06T01:59:23.5203859Z uid=1001(runner) gid=121(docker) groups=121(docker),4(adm),101(systemd-journal)
2021-06-06T01:59:23.5204928Z --- exp	2021-06-06 01:46:14.553974431 +0000
2021-06-06T01:59:23.5205657Z +++ out2	2021-06-06 01:46:14.557974405 +0000
2021-06-06T01:59:23.5206090Z @@ -3,10 +3,10 @@
2021-06-06T01:59:23.5206531Z  id -gn[z] runner: docker
2021-06-06T01:59:23.5207017Z  id -gr[z] runner: 121
2021-06-06T01:59:23.5207747Z  id -grn[z] runner: docker
2021-06-06T01:59:23.5208803Z -id -G[z] runner: 121 4 101
2021-06-06T01:59:23.5209346Z -id -Gn[z] runner: docker adm systemd-journal
2021-06-06T01:59:23.5209885Z -id -Gr[z] runner: 121 4 101
2021-06-06T01:59:23.5210427Z -id -Grn[z] runner: docker adm systemd-journal
2021-06-06T01:59:23.5210966Z +id -G[z] runner: 1214101
2021-06-06T01:59:23.5211545Z +id -Gn[z] runner: dockeradmsystemd-journal
2021-06-06T01:59:23.5212143Z +id -Gr[z] runner: 1214101
2021-06-06T01:59:23.5212729Z +id -Grn[z] runner: dockeradmsystemd-journal
2021-06-06T01:59:23.5213313Z  id -u[z] runner: 1001
2021-06-06T01:59:23.5213752Z  id -un[z] runner: runner
2021-06-06T01:59:23.5214181Z  id -ur[z] runner: 1001
2021-06-06T01:59:23.5214580Z @@ -63,10 +63,10 @@
2021-06-06T01:59:23.5214959Z  id -gn[z] : docker
2021-06-06T01:59:23.5215356Z  id -gr[z] : 121
2021-06-06T01:59:23.5215741Z  id -grn[z] : docker
2021-06-06T01:59:23.5216145Z -id -G[z] : 4 101 121
2021-06-06T01:59:23.5216635Z -id -Gn[z] : adm systemd-journal docker
2021-06-06T01:59:23.5217130Z -id -Gr[z] : 4 101 121
2021-06-06T01:59:23.5217627Z -id -Grn[z] : adm systemd-journal docker
2021-06-06T01:59:23.5218115Z +id -G[z] : 4101121
2021-06-06T01:59:23.5218643Z +id -Gn[z] : admsystemd-journaldocker
2021-06-06T01:59:23.5219177Z +id -Gr[z] : 4101121
2021-06-06T01:59:23.5219707Z +id -Grn[z] : admsystemd-journaldocker
2021-06-06T01:59:23.5220240Z  id -u[z] : 1001
2021-06-06T01:59:23.5220635Z  id -un[z] : runner
2021-06-06T01:59:23.5221032Z  id -ur[z] : 1001
2021-06-06T01:59:23.5221475Z --- gtmp1	2021-06-06 01:46:14.621973984 +0000
2021-06-06T01:59:23.5221932Z +++ gtmp3	2021-06-06 01:46:14.625973958 +0000
2021-06-06T01:59:23.5222482Z @@ -1,8 +1,4 @@
2021-06-06T01:59:23.5222815Z -121 4 101
2021-06-06T01:59:23.5223149Z -
2021-06-06T01:59:23.5223576Z -docker adm systemd-journal
2021-06-06T01:59:23.5223996Z -
2021-06-06T01:59:23.5224320Z -121 4 101
2021-06-06T01:59:23.5224649Z -
2021-06-06T01:59:23.5225245Z -docker adm systemd-journal
2021-06-06T01:59:23.5225979Z -
2021-06-06T01:59:23.5226266Z +1214101 
2021-06-06T01:59:23.5226879Z +dockeradmsystemd-journal 
2021-06-06T01:59:23.5227358Z +1214101 
2021-06-06T01:59:23.5227970Z +dockeradmsystemd-journal 
2021-06-06T01:59:23.5228885Z FAIL tests/id/zero.sh (exit status: 1)

Not sure why ?!

[tertsdiepraam]

Looks like it still expects the items to be separated by spaces when --zero is passed?

[jhscheer]

Looks like it still expects the items to be separated by spaces when --zero is passed?

If I'm interpreting the log right, that's not it.

uu_id --groups uses libc::getgroups to get the groups and then just keeps the inherited order.
However, the problem seems to be that the order is not the same for GNU id and for uu_id.

I already noticed this while writing the tests and needed to use a "work around" test:

// '--groups' ids are in no particular order and when paired with '--zero' there's no
// delimiter which makes the split_whitespace-collect-into-vector comparison impossible.
for opt in &["-G", "--groups"] {
    let args = [opt, z_flag];
    let result = new_ucmd!().args(&args).succeeds().stdout_move_str();
    assert!(!result.contains(" "));
    assert!(result.ends_with('\0'));
}

GNU's id definitely uses some kind of order because it behaves deterministically.
However, the ordering is not obvious to me and I haven't figured out yet what the rules are, e.g. on my local systems:

#GNU (host1)
$ id --groups
1000 10 968 975

#GNU (host2)
$ id --groups
20 501 702 12 61 79 80 81 98 701 33 100 204 250 395 398

e.g. on the CI system (if I'm reading the log right)

#GNU (CI)
$ id --groups
121 4 101

[jhscheer]

This shows how GNU's id changes the order of the IDs returned from the getgroups syscall and uu_id just keeps the order.

$ strace -e getgroups id --groups
getgroups(0, NULL)                      = 4
getgroups(4, [10, 968, 975, 1000])      = 4
1000 10 968 975
+++ exited with 0 +++

$ strace -e getgroups target/debug/id --groups
getgroups(0, NULL)                      = 4
getgroups(4, [10, 968, 975, 1000])      = 4
10 968 975 1000
+++ exited with 0 +++

[tertsdiepraam]

Could it be that GNU always shows the primary group first?

[jhscheer]

Could it be that GNU always shows the primary group first?

Yes, that might be it!
I'll write a fix.

Edit:
Since uu_groups has the same bug, it makes sense to write the fix directly in entries::get_groups(), instead of in id.rs and in groups.rs.

[jhscheer]

@sylvestre @tertsdiepraam
Please have a look at the fix here #2371
If it gets merged, I can complete this PR

[jhscheer]

After the small detour (#2371) to fix the order of the GIDs, the GNU test suite looks much better.
However, there's still something not right:

FAIL: tests/id/zero
===================

uid=1001(runner) gid=121(docker) groups=121(docker),4(adm),101(systemd-journal)
uid=1001(runner) gid=121(docker) groups=121(docker),4(adm),101(systemd-journal)
--- gtmp1  2021-06-08 22:26:37.905457698 +0000
+++ gtmp3  2021-06-08 22:26:37.909457730 +0000
@@ -1,8 +1,4 @@
-121 4 101
-
-docker adm systemd-journal
-
-121 4 101
-
-docker adm systemd-journal
-
+121 4 101
+docker adm systemd-journal
+121 4 101
+docker adm systemd-journal

On the first glance, it looks like newlines are missing (or too much) and that would be easy to fix.
But it's difficult to determine what exactly the problem is because, in order to be able to compare the outputs, tests/id/zero.sh is replacing NULs with spaces and more than one space with a newline (at least that's what I think is happening).

[jhscheer]

This now passes two GNU testsuite Tests that fail on master branch.

============================================================================
Testsuite summary for GNU coreutils 8.32
============================================================================
PASS: tests/id/uid.sh
PASS: tests/id/zero.sh

However, some of the tests I wrote behave differently in the CI environment and need further debugging.

[jhscheer]

I consider GNU coreutils 8.32 the reference version since it's the latest stable release and the GNU Testsuite runs against it.
Since I make extensive use of gathering reference values for stdout/stderr/exitcode by running the coreutils id that is in $PATH, the tests were having problems with subtle differences between versions.
For example:
On ubuntu-16.04 the version is 8.25 and the test would show this (there were a lot more that I didn't track down):

+ id: ‘foobar’: no such user
- id: 'foobar': no such user

Therefore, I implemented to check the version before every invocation of the system id and skip the test if it doesn't match 8.32.
Problem: Now none of the currently deployed ubuntu images run the tests for id since they are all older!
Only on macOS the coreutils are up to date (thanks to brew install).

Does anybody know how feasible it is to run the up-to-date coreutils, that were compiled for the GNU Testsuite, from inside the rust tests (test_id.rs)?

[tertsdiepraam]

I think that's an issue related to the locale, not the coreutils version. Ultimately, we'll have to support different locales, but I think we can focus on the C locale for now. You can set it like this:

❯ id foobar
id: ‘foobar’: no such user
❯ LC_ALL=C id foobar
id: 'foobar': no such user

Or set it as an environment variable.

[jhscheer]

I think that's an issue related to the locale, not the coreutils version.

Interesting, I haven't thought about that and definitely need to keep that in mind and investigate further.
Strange that this happens although the system binaries in my tests are invoked like this: cmd_keepenv(&util_name).env("LANGUAGE", "C").

However, there are other things besides issues related to LOCALE, for example I notice that on 8.30 id doesn't support multiple users, but on 8.32 it does.

# 8.30
$ id root nobody
id: extra operand ‘nobody’
Try 'id --help' for more information.
# 8.32
$ id root nobody
uid=0(root) gid=0(root) groups=0(root)
uid=65534(nobody) gid=65534(nobody) groups=65534(nobody)

[tertsdiepraam]

Interesting, I think it's definitely worthwhile to keep testing against the latest versions. Maybe we could make use an environment variable that points to the GNU utils for the tests? Or we use a ppa in the Ubuntu CI? Maybe this one: https://code.launchpad.net/~dns/+archive/ubuntu/gnu?

Edit: Nevermind about that PPA, it seems unmaintained

[tertsdiepraam]

I know we've had a discussion about running GNU utils in tests before and it's fine if you get this all to work. However, I would suggest changing the tests to use the GNU utils significantly less, maybe with only a few edge cases covered by GNU. For example:

• make more functions like whoami for getting the uid, groups etc. of a user, but keep them really simple (so for example no reordering of the group list)
• test the output format with regex
• test whether all the necessary id's are present (and check if they are in the right position only if possible)

It might not ensure 100% compatibility with GNU, but that's what the GNU tests are for, I guess.

[jhscheer]

Well, my main motivation for these extensive tests was to find all the bugs to get GNU Testsuite tests/id/uid.sh and tests/id/zero.sh to pass. Which worked out well! I couldn't have found all the bugs from just the little feedback provided by tests/id/uid.sh and tests/id/zero.sh alone.

Now that the tests are there, why not use them on the CICD images that have 8.30/8.32, i.e. ubuntu-latest/macOS? (The multiple user feature, only available on 8.32, would need skipping until there's a new linux image with more up-to-date package repos.)
Note that I already implemented the ability to automatically skip tests if coreutils is not present or the version is too low. This also means, that others could run the tests locally without hiccups (i.e. your glitch involving shadow utils).

I don't disagree with the kind of tests you suggest, if the initial situation would be that there are not tests. However, now I don't feel too motivated to implement them because it would mean to invest additional work to change something that tests thoroughly into something that tests less thoroughly.

Generally, I feel like the testing for uutils should involve more usage of coreutils 8.32 for gathering reference values, not less.

[tertsdiepraam]

Which worked out well! I couldn't have found all the bugs from just the little feedback provided by tests/id/uid.sh and tests/id/zero.sh alone.

That's awesome!

However, now I don't feel too motivated to implement them because it would mean to invest additional work to change something that tests thoroughly into something that tests less thoroughly.

I fully understand! There's no point in throwing away perfectly good tests. I was trying to come up with solutions to run them on all platforms, but if you think that's not needed then it's fine.

automatically skip tests if coreutils is not present or the version is too low.

This is more a workaround than a fix, but that's okay given all the different platforms we're trying to support.

Thanks again for your hard work on this!