-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
du: Fix overlapping input arguments #9211
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
base: main
Are you sure you want to change the base?
Conversation
|
GNU testsuite comparison: |
1 similar comment
|
GNU testsuite comparison: |
tests/by-util/test_du.rs
Outdated
| fn du_summary_total_mega_duplicates(s: &str) { | ||
| // TODO: Add checks for non-Linux platforms | ||
| assert_eq!(s, "5\tdir1\n10\t.\n15\ttotal\n"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function causes a "dead code" warning on mac and Windows. To avoid the warning, I would either use the same cfg as you use for test_du_summary_total_mega_duplicates or move the assert_eq! to the test function and remove du_summary_total_mega_duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICS this test should run fine on mac too ? And it could work for Windows if we find a replacement for /dev/urandom ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmh, after work I can install WSL1 and 2 on my windows dual boot and try the output there
Not sure about mac and bsd, looking at other du tests they have some quirks at reporting different sizes compared to linux but I'll search around a bit and try to get the concrete numbers
As for /dev/urandom, I can see if rust fs using writefile could be leveraged to fill the file with random contents or hell it doesn't even have to be random, can scale down to 300 Bytes just to make sure duplicates are not counted. Rust should, similar to cpp/c, have a random generator in std, too
Alright, after work I'll see to finish these tests, thanks guys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using --apparent-size might simplify things: no need to worry about filesystem differences, compression,etc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent idea, I have added it.
Let's see what the CICD will say about other platforms
|
GNU testsuite comparison: |
|
That ubuntu-latest, x86_64-unknown-linux-musl failure is strange, |
|
I will check it locally, may be due to musl. Maybe it's using a different size, like 1 MB = 1000 KB, instead of 1MB = 1024 KB (or the MiB and kiB units), so then it gets something like 5.1 MB and rounds it to 6 or something similar. Intuitively I'd guess that's what's happening, I have ubuntu locally so I will try out with musl to check if that's what's going on Should do a Bytes size and then a MB size test to see reporting differences |
| .unwrap_or_else(|e| panic!("Couldn't write {name}: {e}")); | ||
| } | ||
|
|
||
| pub fn fill_bytes(&self, name: &str, size_bytes: usize, use_rng: bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we need to extend uutests for this.
could you please find another way? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay if I move those utility functions to du tests file?
| } | ||
|
|
||
| /// Using an empty fixture | ||
| pub fn new_fresh<T>(util_name: T) -> Self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay if I move those utility functions locally to du tests file?
@sylvestre
sylvestre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't make modifications on uutests or this
|
Sorry for the hiatus, mononucleosis and life got complicated for the past 2 months |
…arent directory in the arguments Make existing inode tracking not work per-file, but rather on the whole file set, so as to avoid duplicates having their sizes counted twice. Issue: uutils#9202
Test case for when a subdirectory is included with its parent directory in the arguments. Test case scenario provided by: @fedkad Issue: uutils#9202
Remove non-linux test cfgs, since I do not have access to all those platforms. Issue: uutils#9202
Fill_bytes function allows the tester to create a file of a certain size and fill it either randomly, or full of b'a'. Mostly useful for disk usage testing Also add supporting public utility function outside the class itself Issue: uutils#9202
Add option --apparent-size, which hopefully should make the test pass on all platforms, since this option should not report physical disk usage, but rather the apparent/logical disk usage Issue: uutils#9202
1f89934 to
6f23a14
Compare
|
GNU testsuite comparison: |
Fix overlapping input arguments, e.g. when a subdirectory is included with its parent directory in the arguments
Provide test case (scenario by: @fedkad)
Also provide a utility empty fixture constructor for the TestScenario class. Reuse code for the default constructor