Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #666 +/- ##
=======================================
Coverage 19.19% 19.19%
=======================================
Files 68 68
Lines 10135 10135
Branches 545 545
=======================================
Hits 1945 1945
Misses 8190 8190
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Can you fix the tests? |
e28cc66 to
5aaa4cd
Compare
|
Tests are fixed now, but I'm a bit confused by the coverage check. Is there anything I can do to make it come up as successful? |
|
I feel the current test coverage is not very important, and it's already sufficient :) |
src/uu/w/src/w.rs
Outdated
| fn test_fetch_cmdline() { | ||
| // uucore's utmpx returns an i32, so we cast to that to mimic it. | ||
| let pid = process::id() as i32; | ||
| let path = Path::new("/proc").join(pid.to_string()).join("cmdline"); | ||
| assert_eq!( | ||
| fs::read_to_string(path).unwrap(), | ||
| fetch_cmdline(pid).unwrap() | ||
| ); | ||
| let cmdline = std::env::args().collect::<Vec<String>>().join(" "); | ||
| assert_eq!(cmdline, fetch_cmdline(pid)); |
There was a problem hiding this comment.
Could you tell me why it needs to be changed to this?
There was a problem hiding this comment.
The original implementation was basically a 1:1 copy of the code that's inside the fetch_cmdline function (both just build a Path from a PID and read_to_string it).
So previously, we had a test where both sides of the assert_eq are results of basically identical code. This kind of approach guarantees that things "work", but does not test for correctness. Both sides could be outputting the same incorrect result, but the test would pass. You can probably see how that's potentially problematic 😅
Since we're already using std::process::id() to figure out the PID of the process, I thought it would make more sense to compare the result of our fetch_cmdline with a "known correct" value which in this case would be std::env::args(). Hope my explanation makes sense 😅
|
Can you add a comment for the https://man7.org/linux/man-pages/man5/proc_pid_cmdline.5.html |
|
Great work, I think this work can inspire us to consider similar issues elsewhere :) |
|
@Krysztal112233 Thanks for the thorough review. My Rust is... rusty, so these pointers are very much needed. |
6089ea1 to
c1f9af2
Compare
c1f9af2 to
45fbba4
Compare
The command line value exposed by the kernel in
/proc/<pid>/cmdlinerepresents whitespace as NULL bytes.Using
read_to_string()on this directly will cause the output to join all parts of the original command into one long string.This PR adds a very simple fix to avoid this.
Before:
After: