hashsum: test b3sum::test_nonames for real

[huijeong-kim]

Resolves #3818

Signed-off-by: Huijeong Kim [email protected]

[tertsdiepraam]

I think checking whether it is Ok is not enough. try_contains_id returns Ok when the id is valid (i.e. there is an Arg in Command with that id). We also need it to be present, so Ok(true), otherwise nonames can be true when an algorithm other than b3sum is used (I put a dbg!(nonames) on line 305):

❯ cargo run -- hashsum --b2sum --no-names
[src/uu/hashsum/src/hashsum.rs:305] nonames = true

Note that we can assume that b3sum is a valid id, so matches.contains_id("b3sum) should be good enough.

[huijeong-kim]

Oh, thanks! I didn't tested that part.

I used try_contains_id because it panics in test_hashsum::test_check_sha1 due to

thread 'test_hashsum::test_check_sha1' panicked at 'Command was expected to succeed.
stdout =
 stderr = thread 'main' panicked at 'Mismatch between definition and access of `b3sum`. Unknown argument or group id.  Make sure you are using the argument id and not the short or long flags

I thought about using try_contains_id("b3sum").unwrap_or(false), but when I run cargo run -- hashsum --b2sum --no-names nonames is false and it's unexpected.

If nonames should be set for all other algorithms according to the no-names argument, how about this?

let nonames = matches.try_contains_id("no-names").

Or could you suggest any other options that I can take?

[tertsdiepraam]

Oh I see! No names should be false for all other algorithms, so try_contains_id is correct then but we need matches.try_contains_id("b3sum") == Some(true).

[huijeong-kim]

Okay! Thanks! I'll work on it 😃👍🏻

[huijeong-kim]

I used unwrap_or(false) as the return type of try_contains_id is Result<bool, MatchesError>

[tertsdiepraam]

Okay, so a funny thing happened because we merged clap 4. Now, --b3sum is marked with ArgAction::SetTrue and clap has the (questionable) behaviour contains_id is always true for a flag with SetTrue. So this happens again:

❯ cargo run -- hashsum --b2sum --no-names
[src/uu/hashsum/src/hashsum.rs:305] nonames = true

But, that might not be a problem, because we don't actually need nonames to be false for the other algorithms, we just don't care (although we might want to show an error that it's useless with other algorithms). What we actually about is that we can only check the flag if --no-names is registered as an argument (otherwise clap panics). Your solution does this correctly, however, it is not quite obvious what it actually checks. Therefore, I propose the following:

let nonames = *matches.get_one("no-names").unwrap_or(&false);

Sorry for introducing the weird behaviour with clap and changing my mind about this :)

[huijeong-kim]

Oh.. that's weird. No problem, I will change as you suggested!

[huijeong-kim]

While I change my code, I found another weird behavior of clap.
get_one("no-names") panics inside the function call if the flag not exist (in test_check_sha1 test), so that it can't handle unwrap_or().

So I used try_get_one() and it returns Result<Option<T>, MatchesError>. That's why I have two unwraps here..

[tertsdiepraam]

Oh yeah it should have been try_get_one! I made a mistake indeed :)

[sylvestre]

@tertsdiepraam are we good here? thanks

[tertsdiepraam]

Yeah, looks good! Thanks @huijeong-kim!

[huijeong-kim]

Thank you for your kind review @tertsdiepraam 😄👍

[sylvestre]

Terts is the best! ;)

[tertsdiepraam]

That's very kind 😄 Some GNU tests seem to be failing when after merged this: https://github.com/uutils/coreutils/actions/runs/3302752256. I can't reproduce the failure locally though. I'll check whether the issue persists in other runs.

[huijeong-kim]

Oh it fails 🥲 I'll try to find out something from logs..

How can I run the test locally?