It allows the test engine to be paused right before the test cleanup
routine to help with test debugging.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Whoa, nice. :)
What happens if tests are running in parallel? The behaviour I would naively expect is that other running tests will keep going, but new ones will not be scheduled until kyua is resumed.
| contrib/kyua/cli/cmd_debug.cpp | ||
|---|---|---|
| 92 | Is it possible to also print the path to the test's workdir? | |
Comment Actions
Well, this is for starters only :). It logically extends existing "kyua debug" sub-command which is designed to debug a single test case. So, this is for the cases when the annoying failed test is known or a test is under development. Let's see how this one goes.
It is expected to be another bigger patch for the cases when it is unknown beforehand, i.e. we want to run our usual "kyua test ..." with a new flag like the same "-p" to get it stuck upon any failed test. And the parallelism will require discussion, and extra care if we need maximum comfort.
| contrib/kyua/cli/cmd_debug.cpp | ||
|---|---|---|
| 92 | Sure, this is an obvious thing to have. | |
Comment Actions
- Could you please verify that this doesn't regress Linux/MacOS?
- Something like using debugger_ptr_t = std::shared_ptr<debugger> then converting all of the code to use debugger_ptr_t (or maybe just debugger_ptr? I forget the style ATF/kyua uses since I flushed that out of my cache in favor of other garbage :D..) seems like a good idea for clarity/brevity.
I actually have a different proposal.. what if the "automatic cleanup" functionality was opt-out and enabled by default or by a custom policy, e.g.,
- I want the results from the passed test cases,
- I want the results from the failed test cases only, or
- I don't care, clean it up when you're done.
Also, I think that it's honestly better to get the process core and continue on personally. Otherwise you could have resources tied up waiting for a human to push a button.
| contrib/kyua/cli/cmd_debug.cpp | ||
|---|---|---|
| 94–95 | This kind of behavior is not documented in the proposed manpage change. Please thoroughly document the feature so it doesn't confuse folks about the behavior might be like. Also, this kind of thing seems like it would be best driven via kyua.conf (for starters), but the user should be able to override it on the CLI. | |
| contrib/kyua/engine/debugger.hpp | ||
| 43 | Drive-by comment: I wonder if this was something @jmmv did because std::optional didn't exist at the time this was written... | |
Comment Actions
I love this.
It'd be very nice to have the actual output before it pauses, but this is a very nice improvement already, and it's not hard to run the test without '-p' first. That's one of the nice things about having tests, that it's very easy to reproduce specific scenarios.
I'm insufficiently familiar with kyua to offer much detailed feedback on the implementation, other than "I love this feature".
| contrib/kyua/model/test_case.cpp | ||
|---|---|---|
| 241–244 | Could you please use a more descriptive name than d? Also, writing a variable and having const on the method seems a bit odd; I'm kind of surprised clang didn't gripe about it being an issue, but then again it's probably because this method manages a different object instance than _pimpl points to (_pimpl was global state, right? It's been a while since I looked at that code..). Generally speaking something modifying object state shouldn't be using const. | |
Comment Actions
The whole part about processing cores is not really functional outside of FreeBSD with the default kern.corefile sysctl value. MacOS works ok for now since the path to the core file is hacked into Kyua, but Apple likes breaking undocumented stuff in the base OS; Linux requires some extra touch to break OS provided infra because of recent changes made in the Linux kernel 5.x series around managing it via /proc or /sys (it was an odd change). Ubuntu 24.04 and some other distros also install a crash reporter which interferes with how cores are processed.
Comment Actions
I think we're coming at this from different perspectives. For an atf-c test case I'd agree with you that just dumping core for post-mortem debugging with lldb would be more useful.
The use case I have in mind is atf-sh, and specifically the pf tests where we often build setups with multiple vnet jails so we can send traffic between them. In that case being able to access the jails to manually repeat the traffic, run tcpdump, look at state tables, attach dtrace, ... are very common debugging steps. In that scenario keeping the jails running is much more useful.
| contrib/kyua/model/test_case.cpp | ||
|---|---|---|
| 241–244 | Right, const on the method just says it won't modify this directly, not that it won't modify global state (or something via a pointer that belongs to this) | |
Comment Actions
I usually check FreeBSD and Linux, thanks for reminding about macOS build testing.
- Something like using debugger_ptr_t = std::shared_ptr<debugger> then converting all of the code to use debugger_ptr_t (or maybe just debugger_ptr? I forget the style ATF/kyua uses since I flushed that out of my cache in favor of other garbage :D..) seems like a good idea for clarity/brevity.
Sure, we already have things like test_program_ptr, so that debugger_ptr seems to be our option.
I actually have a different proposal.. what if the "automatic cleanup" functionality was opt-out and enabled by default or by a custom policy, e.g.,
- I want the results from the passed test cases,
- I want the results from the failed test cases only, or
- I don't care, clean it up when you're done.
Also, I think that it's honestly better to get the process core and continue on personally. Otherwise you could have resources tied up waiting for a human to push a button.
Yeah, as others commented the intention is a little bit different and related to system/e2e tests or so. I've reflected this in the man page for clarity.
Yes, as I mentioned in Slack it is a known issue -- the stdout/stderr are printed only after that, by current design. I propose to think of this as a separate patch due to it can be disputable and it may postpone this already useful feature.
| contrib/kyua/cli/cmd_debug.cpp | ||
|---|---|---|
| 94–95 | You got me, I was thinking exactly about the same but was rushing to the working patch first :) Now the man page is updated to mention that such way it becomes interactive, Regarding the kyua.conf. As long as it's just an ad-hoc option for a single test case debugging there are no much use cases for having it coming from the config file. | |
| contrib/kyua/engine/debugger.hpp | ||
| 43 | Yes, Kyua has its own optional. | |
| contrib/kyua/model/test_case.cpp | ||
| 241–244 |
Sure.
Well, it is intentional :) The encapsulation is not an easy part of software design, you never know where the project is going to be tomorrow. I worked with big open-source OOP-based projects with famous companies behind when something was hidden or sealed by design with good reasoning and later users find this like a pain when a trivial fix or a simple extension requires extra effort and workarounds to apply. Here we have a test case concept which is immutable-like after creation by design, and test_program::find() as the main provider yields const. I decided not to touch this long standing design part. That's why the method is called attach_debugger instead of a usual setter name like set_debugger to emphasize that it's not about changing the test case itself. But yes, from academical perspective it should not be done like this. What do you think? | |
Comment Actions
This feature can be refined, but I don’t want to get in the way of the utility of this change for folks like @kp, who need the debuggability. Let’s go forward with this change and refine things/sand down rough edges on GitHub.
| contrib/kyua/drivers/debug_test.cpp | ||
|---|---|---|
| 96 | What if the value returned is ‘.end()’ (not found)? | |
| contrib/kyua/drivers/debug_test.cpp | ||
|---|---|---|
| 96 | Good point, and the current design is that the find() itself would through an exception. Actually, if the flow reached this point then it is not expected to be a case. | |
| contrib/kyua/drivers/debug_test.cpp | ||
|---|---|---|
| 96 | throw* | |
Comment Actions
Thanks for your time. Deal. This patch will land in the coming days. I've opened the respective GitHub PR to continue with internals there: https://github.com/freebsd/kyua/pull/266.
| contrib/kyua/doc/kyua-debug.1.in | ||
|---|---|---|
| 101 | Many thanks for the help. | |
Comment Actions
A couple more man page nits, otherwise this seems fine to me, modulo the other comment. I didn't carefully review the code changes.
| contrib/kyua/doc/kyua-debug.1.in | ||
|---|---|---|
| 90 | Or, "Causes kyua to pause right before ..." | |
| 91 | ||
| 92 | I missed this constraint last time I read through this. Is it possible to have this work even if there is no cleanup routine? The reason is that sometimes there are useful artifacts left in the test's sandbox directory (I think this is usually /tmp/kyua.XXXXXX), and I often want to have access to it when debugging. No cleanup routine is needed to remove that directory since kyua handles it automatically. I don't mean for this comment to block the patch, since the functionality is obviously useful. Sorry for bringing it up so late. | |
| contrib/kyua/doc/kyua-debug.1.in | ||
|---|---|---|
| 92 | Actually, this is how it must be done. Somehow I ended up focusing mostly on network tests which have explicit test specific artifacts to be removed by their cleanup routines. Many thanks for spotting this. | |