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
flaky test: TestPromptForConfirmation/case=reader_closed #4948
Comments
cc @Benehiko |
Good point from @tonistiigi here – #4888 (comment). A more holistic approach would be to always set up signal handling in the CLI, and use it to cancel the command's context, and just migrate most places in the codebase that are setting up their own signal handling to instead handle context cancellation – funny that we do this for plugins but not for anything else. |
Ah, yes, I saw that comment, and wanted to ask @krissetto about it as well.
Sounds like a sensible approach yes (or at least, that's my first response; contexts are always fun) |
I think this can only work once we have wired up |
Good catch. I agree with always setting up a signal handler in the CLI and cancelling with the context, but i'm not sure how many changes it'd require in the codebase ATM to pass the context everywhere it might be needed. I think what confused me was this part of the
I thought that once a signal got consumed, the original behavior would be restored (so the default signal handling behavior of the application). That said, I think this specific panic is only related to the test, as the code itself doesn't explicitly close any channels (the Particularly this part at lines 205-210 of
|
Oh! I think I meant to |
Looking at this error, I think the timeout might be too little for some machines. I've not been able to reproduce this flake locally on my laptop so the flake might be dependent on the machine running it. I'm guessing the timeout for the context is reached (100 milliseconds) and then the test is ended with This is a tricky one since there is no way to guarantee that closing the reader will exit the prompt in a given time frame since this would be dependent on the OS and machine hardware. I could prevent the panic by not closing the channels inside the test and leaving it up to the garbage collector and hope that an increase to say 500ms or 1 second timeout is enough for most cases. |
Description
This test looks to be flaky on macOS at least; seen it fail a couple of times.
The text was updated successfully, but these errors were encountered: