`tests/tail`: Fix tests to reflect changes from the refactoring #3905 #3965

Joining7943 · 2022-09-21T22:29:38Z

This is a follow up pr to #3905 (refactoring tail), in which the tests for tail are updated to reflect the current state of tail's implementation.

I removed all cfg attributes from the tests which limited the tests by vendor, os (ci 8594), then added cfg attributes again for the tests failing in the ci for a specific target os (ci 8604, ci 8605, ci 8606, ci 8607). This is a quick overview over the outcome and the main differences to before the refactoring:

Piped input: works now for all platforms; worked before for all oses but windows and android
Redirected input: before: linux; after: all but apple
There are also some additional general and --follow tests working
... See diff to main or the summary below

Summary and curated diff from this branch to main

-#[cfg(all(unix, not(any(target_os = "android", target_vendor = "apple"))))]
+#[cfg(not(target_vendor = "apple"))]
test_stdin_redirect_file

-#[cfg(all(unix, not(any(target_os = "android", target_vendor = "apple"))))]
+#[cfg(not(target_vendor = "apple"))]
test_stdin_redirect_offset

 #[test]
-#[cfg(all(unix, not(any(target_os = "android", target_vendor = "apple"))))]
+#[cfg(all(not(target_vendor = "apple"), not(target_os = "windows")))]
test_stdin_redirect_offset2

-#[cfg(all(unix, not(target_os = "freebsd")))]
+#[cfg(unix)]
test_permission_denied

-#[cfg(all(unix, not(target_os = "freebsd")))]
+#[cfg(unix)]
test_permission_denied_multiple
-#[cfg(target_os = "linux")]
test_follow_redirect_stdin_name_retry

-#[cfg(not(target_os = "macos"))]
-#[cfg(all(unix, not(any(target_os = "android", target_os = "freebsd"))))]
+#[cfg(all(
+    not(target_vendor = "apple"),
+    not(target_os = "windows"),
+    not(target_os = "android"),
+    not(target_os = "freebsd")
+))]
test_stdin_redirect_dir

-#[cfg(target_os = "linux")]
test_follow_stdin_descriptor

-#[cfg(target_os = "linux")]
test_follow_stdin_name_retry

-#[cfg(target_os = "linux")]
-#[cfg(disable_until_fixed)]
test_follow_bad_fd

-#[cfg(unix)]
+#[cfg(not(target_os = "windows"))]
test_follow_single

-#[cfg(unix)]
+#[cfg(not(target_os = "windows"))]
test_follow_non_utf8_bytes

-#[cfg(unix)]
+#[cfg(not(target_os = "windows"))]
test_follow_multiple

-#[cfg(unix)]
+#[cfg(not(target_os = "windows"))]
test_follow_name_multiple

-#[cfg(unix)]
test_follow_multiple_untailable

-#[cfg(all(unix, not(target_os = "android")))]
test_follow_stdin_pipe

-#[cfg(unix)]
+#[cfg(not(target_os = "windows"))]
test_follow_invalid_pid

-#[cfg(disable_until_fixed)]
+#[cfg(all(
+    not(target_vendor = "apple"),
+    not(target_os = "windows"),
+    not(target_os = "android")
+))]
test_follow_with_pid

-#[cfg(all(unix, not(target_os = "android")))]
test_bytes_stdin

-#[cfg(all(unix, not(target_os = "android")))]
test_positive_bytes

-#[cfg(all(unix, not(target_os = "android")))]
test_positive_zero_bytes

-#[cfg(all(unix, not(target_os = "android")))]
test_positive_lines

-#[cfg(all(unix, not(target_os = "android")))]
test_obsolete_syntax_positive_lines

-#[cfg(all(unix, not(target_os = "android")))]
test_small_file

-#[cfg(all(unix, not(target_os = "android")))]
test_obsolete_syntax_small_file

-#[cfg(all(unix, not(target_os = "android")))]
test_positive_zero_lines

-#[cfg(all(unix, not(target_os = "android")))]
test_invalid_num

-#[cfg(all(unix, not(target_os = "android")))]
test_num_with_undocumented_sign_bytes

-#[cfg(unix)]
test_retry1

-#[cfg(unix)]
test_retry2

-#[cfg(target_os = "linux")]
+#[cfg(not(target_os = "windows"))]
test_retry6

-#[cfg(all(unix, not(any(target_os = "android", target_vendor = "apple"))))]
+#[cfg(all(not(target_os = "windows"), not(target_os = "freebsd")))]
test_retry9

-#[cfg(target_os = "linux")]
+#[cfg(all(
+    not(target_vendor = "apple"),
+    not(target_os = "windows"),
+    not(target_os = "freebsd")
+))]
test_follow_descriptor_vs_rename1

-#[cfg(target_os = "linux")]
+#[cfg(all(
+    not(target_vendor = "apple"),
+    not(target_os = "windows"),
+    not(target_os = "freebsd")
+))]
test_follow_descriptor_vs_rename2

-#[cfg(all(unix, not(target_os = "android")))]
+#[cfg(all(not(target_os = "windows"), not(target_os = "android")))]
test_follow_name_remove

 #[test]
-#[cfg(target_os = "linux")]
+#[cfg(all(
+    not(target_os = "windows"),
+    not(target_os = "android"),
+    not(target_os = "freebsd")
+))]
test_follow_name_truncate1

-#[cfg(target_os = "linux")]
+#[cfg(all(
+    not(target_os = "windows"),
+    not(target_os = "android"),
+    not(target_os = "freebsd")
+))]
test_follow_name_truncate2

-#[cfg(target_os = "linux")]
+#[cfg(not(target_os = "windows"))]
test_follow_name_truncate3

-#[cfg(unix)]
+#[cfg(not(target_os = "windows"))]
test_follow_name_truncate4

-#[cfg(all(unix, not(target_os = "android")))]
+#[cfg(not(target_os = "windows"))]
test_follow_truncate_fast

-#[cfg(target_os = "linux")]
+#[cfg(all(
+    not(target_vendor = "apple"),
+    not(target_os = "windows"),
+    not(target_os = "freebsd")
+))]
test_follow_name_move_create2

-#[cfg(all(unix, not(target_os = "freebsd")))]
+#[cfg(not(target_os = "windows"))]
test_follow_inotify_only_regular

-#[cfg(unix)]
take_stdout_stderr

-#[cfg(all(unix, not(target_os = "android")))]
test_no_trailing_newline

-#[cfg(all(unix, not(target_os = "android")))]
test_lines_zero_terminated

-#[cfg(all(unix, not(target_os = "android")))]
test_presume_input_pipe_default

-#[cfg(unix)]
+#[cfg(not(windows))]
test_fifo

-#[cfg(all(not(target_os = "android"), not(target_os = "windows")))]
mod pipe_tests

For completeness:
I needed to keep 2 piped input tests for windows switched off which are not working in the ci because the tests timed out. I couldn't reproduce locally why they are hanging and they ran successfully in a Windows 10 Virtualbox image.

Fixes #3881

tertsdiepraam

Looks like you forgot to include the changes in tail.rs? I only see test_tail.rs and random.rs.

tertsdiepraam · 2022-09-23T21:34:41Z

tests/by-util/test_tail.rs

@@ -148,6 +150,7 @@ fn test_stdin_redirect_offset2() {
        .succeeded();
 }

+// TODO: Use a debug_assert() instead


Instead of what? Also debug_assert is not very meaningful in tests. Tests are not included in regular binaries and should probably always fail early if something yas gone wrong, even when run with --release

Sorry, seems my TODOs are only meant for me. This is a quick one about adding a debug_assert() in tail.rs() after the immediate exit because of -[nc]0 without -f instead of trying to provoke an error like passing a missing file to tail. A debug_assert() is more precise in my eyes.

tertsdiepraam · 2022-09-23T21:35:45Z

tests/by-util/test_tail.rs

 #[test]
-#[cfg(all(unix, not(target_os = "freebsd")))]
-fn test_nc_0_wo_follow2() {


Why is this removed?

Because the test is platform specific in contrast to test_nc_0_wo_follow and it doesn't more information. In my eyes the test_nc_0_wo_follow test was sufficient. However, it could be more precise with the debug_assert(). If you like to, I can readd it.

I can't follow your reasoning.

Because the test is platform specific in contrast to test_nc_0_wo_follow

This should not be a reason to remove a test.

and it doesn't more information.

But the two tests check different things.

tertsdiepraam · 2022-09-23T21:37:00Z

tests/by-util/test_tail.rs

+    not(target_os = "windows"),
+    not(target_os = "android"),
+    not(target_os = "freebsd")
+))] // FIXME:


There's a couple of these FIXME: comments. I'd say either give an explanation or remove the colon :)

Sorry, my FIXME parser doesn't colorize these without the colon. Maybe I can change that somewhere in the settings. I hoped the FIXMEs are seen as annotation for the not working platforms.

However, I'll write a short explanation for the standalone FIXMEs :)

tests/by-util/test_tail.rs

tertsdiepraam · 2022-09-23T21:40:56Z

tests/by-util/test_tail.rs

@@ -363,7 +340,6 @@ fn test_follow_stdin_name_retry() {
 }

 #[test]
-#[cfg(target_os = "linux")]


Is this test gonna work on all platforms eventually or can it only be fixed for some platforms?

Good question...

We should probably keep the cfg then, ut was probably added for a reason.

tertsdiepraam · 2022-09-23T21:44:54Z

tests/by-util/test_tail.rs

    new_ucmd!()
        .args(&["-c", "1Y", "emptyfile.txt"])
        .fails()
        .stderr_str()
        .starts_with("tail: invalid number of bytes: '1Y': Value too large for defined data type");
-    #[cfg(not(target_pointer_width = "128"))]


Feels like we should definitely keep these? CI can't cover every possible arch and OS we want to run the tests on

tertsdiepraam · 2022-09-23T21:50:04Z

tests/common/random.rs

@@ -310,5 +310,25 @@ mod tests {
            random_string.as_bytes().iter().filter(|p| **p == 0).count()
        );
        assert!(random_string.as_bytes().ends_with(&[0]));
+
+        // This test exists to exclude an error (timeout) occurred during system tests on windows


How does it do this?

It does so because this is the exact code from the test in which I used the random generator and the test timed out. But there's more code in the test, so to exclude the possibility that it has something to do with the random string generator, I added these lines from the test here and test them separately.

Hmm, ok, I'm not 100% if this is useful, but I think it's a vit better to at least add that explanation to the code here

Joining7943 · 2022-09-23T21:57:27Z

Looks like you forgot to include the changes in tail.rs? I only see test_tail.rs and random.rs.

I didn't change anything in tail.rs since the refactoring.

Joining7943 · 2022-09-24T00:23:21Z

@tertsdiepraam A little addition to my last comment. I'm not sure if my explanation in the header of this pr was a little bit short. This pr doesn't change any production code. I've already done this in the refactoring pr. I'm kind of summarizing in this pr, what was achieved in the refactoring pr #3905.

tertsdiepraam · 2022-09-24T05:53:45Z

I see. That makes sense, but indeed wasn't totally clear to me from the title and description.

Joining7943 · 2022-09-24T22:13:56Z

@tertsdiepraam I updated the title and description

tertsdiepraam

I generally like this change. Enabling more tests is always good. I have two more small comments and please put the removed test back from the last review.

tertsdiepraam · 2022-09-27T22:27:18Z

tests/by-util/test_tail.rs

+// TODO: Use a debug_assert() in tail.rs, which is more precise, to verify we're
+// exiting immediately instead of the approach below.


Please remove this TODO

tests/by-util/test_tail.rs

Joining7943 · 2022-09-28T04:22:41Z

I generally like this change.

Thanks :) and thanks for the review.

Enabling more tests is always good.

Generally, I like more tests, too. But here, I also wanted to cement what we gained so far.

I have two more small comments and please put the removed test back from the last review.

Concerning the TODO and tests, please let me propose an alternative solution, which improves these tests a little bit. Originally, I didn't want to alter production files in this pr, so I kept this TODO. However, I added an additional commit, so you can see, what I meant with the TODO.

jhscheer · 2022-09-28T12:00:23Z

Concerning the TODO and tests, please let me propose an alternative solution, which improves these tests a little bit.

Please do not remove the two nc_0_wo_follow tests, they're there to ensure we don't regress and fail gnu/tests/tail-2/tail-n0f.sh.

Joining7943 · 2022-09-29T20:27:41Z

@tertsdiepraam Did you already have a look at the last commit?

tertsdiepraam · 2022-09-29T20:54:03Z

Sorry, I don't like the debug_assert approach. One reason is that the assert is disjointed from the actual testing code, making it so that it's easily removed by an unsuspecting contributor. Secondly, tests can also be run in release mode in which case the debug_assert is removed. What was wrong with the original test? It also feels like it's just testing whether if expressions and std::process::exit work? It's reusing the logic within tail, making the test less meaningful if that makes sense.

Joining7943 · 2022-09-29T21:15:03Z

@tertsdiepraam No problem.

Joining7943 · 2022-10-01T08:28:03Z

@tertsdiepraam I took care of all your comments. The macos timeout looks like a ci problem. All other tests on macos have been running successfully...

jhscheer

Looks good to me.
Please consider my suggestions.

Thanks for all the extensive testing on the various platforms.

jhscheer · 2022-10-02T12:08:54Z

tests/by-util/test_tail.rs

@@ -1512,7 +1540,7 @@ fn test_retry8() {
 }

 #[test]
-#[cfg(all(unix, not(any(target_os = "android", target_vendor = "apple"))))] // FIXME: make this work not just on Linux
+#[cfg(all(not(target_os = "windows"), not(target_os = "freebsd")))] // FIXME: for currently not working platforms


This test is problematic. It has been observed to sometimes fail on MinRustV and now macOS.
#3929
Until this is fixed I would not relax the cfg.

Are you aware that this comes across as quite rude? Why is this a won't fix? @jhscheer brings up a valid problem with your PR, so that needs to be fixed before we can merge this. I'm unresolving this conversation until it is actually resolved.

It's a won't fix because this is actually valuable information about the intermittenly failing tests. Additionally, these intermittently failing tests are not part of this pr nor the refactoring on which this pr is based on. You may wish to disable the test until it is fixed, but I would suggest another pr for this.

But this PR enables the test on mac, which is not correct. So this PR should not introduce that change in the first place.

The retry9 test is failing intermittently, but not because of the refactoring pr. It also does on macos but now you can notice this fact and maybe it helps figuring out the problem. The test usually passes and if not then because of #3929.

We try to keep intermittent failure to a minimum, because it affects everyone's CI for PRs. We can't accept PRs that knowingly introduce more intermittent failures.

but not because of the refactoring pr.

The refactoring PR has nothing to do with this. There is a test that fails (intermittently) and this PR enables it, which is a problem and that is why this PR has to just not do that.

It also does on macos but now you can notice this fact and maybe it helps figuring out the problem.

We now indeed know this and that is helpful, but that's not a valid reason that we should ignore the problem here. We're not asking you to fix anything, just to revert the change that you made for this specific test.

Not meant badly, I'm not sure this is going into the right direction, but if dsabling this test on macos is all you want, it's done.

jhscheer · 2022-10-02T12:25:48Z

tests/by-util/test_tail.rs

+// FIXME: windows: this test failed with timeout in the CI. Running this test in
+// a Windows VirtualBox image produces no errors.
+#[test]
+#[cfg(not(target_os = "windows"))]


In case a test runs locally but not in the CI, I would use is_ci(), instead of disabling the test completely.
e.g.

if cfg!(windows) && is_ci() { println!("TEST SKIPPED (<reason>)"); return; }

Please explain why "Won't fix"

I won't change it, because it's correct the way it is. It clearly indicates the implementation has a problem on windows. The comment just explains what I've found out and is a help for others. Sure it's failing in the ci and passing on my pc and my virtualbox but that doesn't mean there's something wrong with the implementation for windows, so it is switched off entirely. Windows pipes are different from unix pipes and fixing this in this pr is out of the scope of this pr. Additionally, if someone's running the test suite on windows but working on uutils in a different place he may probably run in this timeout.

@jhscheer ok with you?

jhscheer · 2022-10-02T12:28:01Z

tests/by-util/test_tail.rs

+// FIXME: windows: this test failed with timeout in the CI. Running this test in
+// a Windows VirtualBox image produces no errors.
+#[test]
+#[cfg(not(target_os = "windows"))]


See comment about is_ci() above.

…3905

Joining7943 changed the title ~~Fix piped and redirected input for windows~~ tail: Fix piped and redirected input for windows Sep 21, 2022

Joining7943 force-pushed the fix-windows-pipe branch from b76af7f to 7b354a2 Compare September 22, 2022 19:43

Joining7943 marked this pull request as ready for review September 23, 2022 21:11

Joining7943 changed the title ~~tail: Fix piped and redirected input for windows~~ tail: Fix piped and redirected input and other functionality Sep 23, 2022

tertsdiepraam reviewed Sep 23, 2022

View reviewed changes

Joining7943 changed the title ~~tail: Fix piped and redirected input and other functionality~~ tests/tail: Fix tests to reflect changes from the refactoring #3905 Sep 24, 2022

Joining7943 force-pushed the fix-windows-pipe branch from 2e86269 to c6a2d5d Compare September 27, 2022 17:10

tertsdiepraam reviewed Sep 27, 2022

View reviewed changes

Joining7943 force-pushed the fix-windows-pipe branch from c6a2d5d to 8f80def Compare September 28, 2022 04:20

Joining7943 force-pushed the fix-windows-pipe branch from 8f80def to 41226ba Compare September 29, 2022 21:24

tertsdiepraam requested a review from jhscheer October 1, 2022 12:10

Joining7943 force-pushed the fix-windows-pipe branch from 41226ba to 6485f58 Compare October 1, 2022 20:17

jhscheer suggested changes Oct 2, 2022

View reviewed changes

Joining7943 force-pushed the fix-windows-pipe branch 3 times, most recently from 8a4416a to 0897bbc Compare October 4, 2022 04:57

tests/tail: Fix tests to reflect changes from the refactoring uutils#…

1725151

…3905

sylvestre force-pushed the fix-windows-pipe branch from 0897bbc to 1725151 Compare October 5, 2022 11:32

sylvestre merged commit 97f3dfa into uutils:main Oct 6, 2022

Joining7943 deleted the fix-windows-pipe branch January 2, 2023 09:28

		// TODO: Use a debug_assert() in tail.rs, which is more precise, to verify we're
		// exiting immediately instead of the approach below.

Uh oh!

tests/tail: Fix tests to reflect changes from the refactoring #3905 #3965

tests/tail: Fix tests to reflect changes from the refactoring #3905 #3965

Uh oh!

Conversation

Joining7943 commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joining7943 Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joining7943 Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joining7943 commented Sep 23, 2022

Uh oh!

Joining7943 commented Sep 24, 2022

Uh oh!

tertsdiepraam commented Sep 24, 2022

Uh oh!

Joining7943 commented Sep 24, 2022

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Joining7943 commented Sep 28, 2022

Uh oh!

jhscheer commented Sep 28, 2022

Uh oh!

Joining7943 commented Sep 29, 2022

Uh oh!

tertsdiepraam commented Sep 29, 2022

Uh oh!

Joining7943 commented Sep 29, 2022

Uh oh!

Joining7943 commented Oct 1, 2022

Uh oh!

jhscheer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

`tests/tail`: Fix tests to reflect changes from the refactoring #3905 #3965

`tests/tail`: Fix tests to reflect changes from the refactoring #3905 #3965

Joining7943 commented Sep 21, 2022 •

edited

Loading

Joining7943 Sep 23, 2022 •

edited

Loading

Joining7943 Sep 23, 2022 •

edited

Loading

tertsdiepraam Oct 2, 2022 •

edited

Loading

Joining7943 Oct 4, 2022 •

edited

Loading