-
Notifications
You must be signed in to change notification settings - Fork 210
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
Fix --pause-on-fail #2036
Fix --pause-on-fail #2036
Conversation
i mildly prefer deferring the whole post-failure logic like this 8000548 so that we also get printing flows and capturing a sysdump on failure 💭 |
👍 I'm not opinionated at all. That commit doesn't work for me, but happy to try any alternative proposal. |
alright let me try and figure out why it's not working for you 💭 |
sorry i'm going back and forth on this, now i like this better than 8000548. could you update this pr:
|
Hmm, I think it's pausing twice with that proposal:
|
6ef3a6e
to
853632d
Compare
(updated to follow your latest feedback) This first pause is actually not great, because it's pausing before printing the debuggability steps:
|
oh yeah i think we can just delete ace4127#diff-e3c06da3c6d8b3d471002757d5e992af824a412431f22e7f18f5f245f35552adR217. no need to call failCommon() from there 💡 |
Hmm, unfortunately that's the one that is printing after printing the other output that's relevant to debugging the test. Deleting that one will prevent there being double pause, but it also deletes the pause at the most useful time.. |
For what it's worth, I think that this model where individual tests know about the control flow of the overall testing and run |
would it help if we swap these 2 lines => https://github.com/cilium/cilium-cli/pull/2036/files#diff-66a6f82a060f753d91cfb453548d1f41fd695abde4debdb8cc5b982b6b1260beR314-R315 |
Signed-off-by: Joe Stringer <[email protected]>
Move the --pause-on-fail and --collect-sysdump-on-failure flag support into Test.failCommon() and call into there from other common paths. Signed-off-by: Joe Stringer <[email protected]> Signed-off-by: Joe Stringer <[email protected]>
If the user does --pause-on-fail and then does ctrl+c, it should terminate the program rather than waiting for the user to input a line first. Signed-off-by: Joe Stringer <[email protected]>
I'm not sure about the significance of moving the flush until after the log, but swapping the order has the desired effect of seeing the failure first then pausing. |
853632d
to
fa2b1f1
Compare
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.
✨
cilium connectivity test --pause-on-fail
wasn't working before because theindividual tests would fail out of the goroutine and the wrapping logic
wouldn't properly detect and enforce the pause. Add it into the fail/fatal
commands to make sure it takes effect, even if that means blocking an
individual test goroutine on user input.
Fixes: #2033