On Thu, 15 Jun 2023 at 16:20, Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Thu, Jun 15, 2023 at 3:44 AM Boqun Feng boqun.feng@gmail.com wrote:
Great work! I've played this for a while, and it's really useful ;-)
Thanks!
The assertion warning only says line 35 but which file? Yes, the ".._sync_lock_spinlock_rs" name does provide the lead, however since we generate the test code, so we actually know the line # for each real test body, so I come up a way to give us the following:
[..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61 [..] Expected e.c == 11 to be true, but is false [..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0Thoughts?
I like it.
A part of me would like to keep the kernel::kunit::info(format_args!(" # Doctest from line {line}\n"));
If only so we can preserve the line information when the tests actually pass. This is something that'd probably ultimately fit as a "test attribute": https://lore.kernel.org/linux-kselftest/20230610005149.1145665-1-rmoar@googl...
For C tests we're not bothering outputting line numbers now (though again, are considering if we can do it with an attribute), but those tests have much more searchable names, so I think it still makes sense for the Rust ones.
How about printing something like: # source_line: {}
kunit.py will hide this when the test passes unless the user explicitly passes --raw_output.
Sounds good to me. However, David/Philip, is this OK or do you really need/use the actual/compiled source file there? If you don't need it, does it need to exist / be a real file at least? If the latter answer is a "yes", which I guess it may be likely, then:
I don't think there's anything automated using the file:line in assertion messages, it's just for human consumption. This may change in the future (and there are probably some text editors around which will turn a path like that into, e.g., a clickable link now), but we're not currently doing anything which would actually open the file.
let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/"));This would not work for files with a `_` in their name, like `locked_by.rs`. I guess we could still find the real filename based on that information walking the dir, which is another hack I recall considering at some point.
Otherwise, if "fake" filenames in the line above are OK for David/Philip (I suspect they may want to open them for reporting?), then I guess the `file` one may be good enough and eventually we should get `rustdoc` to give us the proper metadata anyway.
Personally, I'd think a "fake filename" is okay (especially if it's temporary until we can get the right one), though I'd prefer there to be some indication that it's "fake": maybe leaving the _ separator in, or wrapping it in brackets, or something? Unless the whole walk-the-filesystem technique ends up being worth doing, so we don't have a significant chance of the filename being dud.
-- David