Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/du: run test_du_time with TZ=UTC. #4446

Merged
merged 1 commit into from
Mar 3, 2023
Merged

Conversation

eggpi
Copy link
Contributor

@eggpi eggpi commented Feb 28, 2023

Hi,

When running tests locally, I noticed that test_du_time fails because it expects the output to be 1h offset from what the tool produces:

$ cargo test --features "du touch" --no-default-features
...
---- test_du::test_du_time stdout ----
run: /Users/guilherme/src/coreutils/target/debug/coreutils touch -a -t 201505150000 date_test
run: /Users/guilherme/src/coreutils/target/debug/coreutils touch -m -t 201606160000 date_test
run: /Users/guilherme/src/coreutils/target/debug/coreutils du --time date_test
thread 'test_du::test_du_time' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
<0	2016-06-16 01:00	date_test
>0	2016-06-16 00:00	date_test


', tests/by-util/test_du.rs:409:12


failures:
    test_du::test_du_time

test result: FAILED. 85 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.82s

I believe the test assumes it is running on a machine with UTC timezone. I am in UTC + 1, which the tool output is correctly aware of. This PR changes the test to also expect local times.

Apologies if this is not very idiomatic, I am fairly new to Rust and obviously open to iterating on this 😄

Thanks in advance for the review!

@eggpi
Copy link
Contributor Author

eggpi commented Feb 28, 2023

Fixed linter errors (hopefully).

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, though it is a bit unfortunate that we have to do it this way, because now everyone can only test their own timezone. It would be better if we could specify the timezone in the test somehow. I wonder if that's possible.

@github-actions
Copy link

github-actions bot commented Mar 1, 2023

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@eggpi
Copy link
Contributor Author

eggpi commented Mar 1, 2023

I could implement support for the TZ environment variable in du and use that to lock the test to UTC?

Testing this on a UTC Linux system:

$ du --version
du (GNU coreutils) 8.30
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Torbjorn Granlund, David MacKenzie, Paul Eggert,
and Jim Meyering.
$ du --time /etc/lsb-release
4	2022-08-22 10:53	/etc/lsb-release
$ TZ=UTC-5 du --time /etc/lsb-release
4	2022-08-22 15:53	/etc/lsb-release

What do you think?

@tertsdiepraam
Copy link
Member

That should actually already work! That's because libc respects that variable and chrono uses libc:

❯ TZ=UTC-5 cargo run -q -- du --time Cargo.toml
20      2023-02-26 18:02        Cargo.toml
❯ cargo run -q -- du --time Cargo.toml
20      2023-02-26 14:02        Cargo.toml

So maybe we run the tests with TZ=UTC? Does that work? We could actually make that a variable that's added by default. Though, we'd have to change some other tests in that case. Let's start by just trying to add it here.

@eggpi
Copy link
Contributor Author

eggpi commented Mar 2, 2023

I've changed this specific test to set TZ=UTC across all of the commands it invokes, I assume that's what you mean as the first step? This does seem to work fine.

@eggpi eggpi changed the title tests/du: make test_du_time aware of local timezone. tests/du: run test_du_time with TZ=UTC. Mar 2, 2023
@tertsdiepraam
Copy link
Member

Nice! That's indeed what I meant.

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@eggpi
Copy link
Contributor Author

eggpi commented Mar 2, 2023

Great! And after another encounter with the linter, this should be good now.

@tertsdiepraam
Copy link
Member

Looks great! Could you add a comment on why it was necessary to add that variable for people who read the code without the context of the issue or this PR?

du --time formats a file's timestamp according to the local timezone,
but the test implicitly assumed UTC. This caused it to fail when
running locally in my UTC+1 machine.

$ cargo test --features "du touch" --no-default-features
Failure: https://gist.github.com/eggpi/651e01559b7c59e9457c1b22fe4c0c19
@eggpi
Copy link
Contributor Author

eggpi commented Mar 2, 2023

Sure thing, done!

@sylvestre sylvestre merged commit 60771b1 into uutils:main Mar 3, 2023
@sylvestre
Copy link
Contributor

thanks

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