Add tests calling terminating signals to func-e #372
Conversation
This is a follow-up for #371 (review) to test all possible signals to func-e. A minor change on checking the stderr: this uses scanner instead of peek on the buffer since for some reason redirecting cmd.Stderr to a buffer doesn't give complete stderr after the process is terminated. Hence here we use scanner to io.ReadCloser of cmd.StderrPipe() instead. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
|
Seems like adding more parallel tests gives Windows a hard time? |
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
| stop() | ||
|
|
||
| // Simulate handleShutdown like in envoy.Run. | ||
| _ = moreos.Interrupt(cmd.Process) |
There was a problem hiding this comment.
Probably this one causing -1073741510 on Windows? If we don't have this, we waste 5 secs waiting for the child process to terminate.
There was a problem hiding this comment.
This is because I didn't set the cmd.SysProcAttr for windows.
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
| requireFindProcessError(t, fakeEnvoyProcess, process.ErrorProcessNotRunning) | ||
| tc.signal(cmd.Process) | ||
| if tc.waitForExiting { | ||
| requireScannedWaitFor(t, stderrScanner, "exiting") |
There was a problem hiding this comment.
maybe adding comment about where "exiting" comes from would help future contributor :D
There was a problem hiding this comment.
Thanks. A good call. I added it.
Signed-off-by: Dhi Aurrahman <dio@rockybars.com> Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>
This addresses some fuzz to make sure the comments don't lie. Notably, this defers contexts the same way between fake and real. This also pulls out handleShutdown to accent notes from #372 Signed-off-by: Adrian Cole <adrian@tetrate.io>
| @@ -136,3 +180,14 @@ func requireFindProcessError(t *testing.T, proc *os.Process, expectedErr error) | |||
| return err == expectedErr | |||
| }, 100*time.Millisecond, 5*time.Millisecond, "timeout waiting for expected error %v", expectedErr) | |||
| } | |||
|
|
|||
| func requireScannedWaitFor(t *testing.T, s *bufio.Scanner, waitFor string) { | |||
There was a problem hiding this comment.
later I will make a refactoring PR as we use scanner in the e2e for the same reasons, even if it could be documented better. Most notably we also avoid require.Eventually as it makes crap error messages when it fails. In other words, we used to use require.Eventually, that caused very difficult troubleshooting in CI, then we switched off of it.
Even if we fail to do refactoring for whatever reason, when there is copy/paste or something very similar, it is worth commenting the other function or file this is almost the same as. That way we can know to look at it in worst case. Folks who read those comments can help prevent adding a third similar thing later. It also helps reviewers who may have forgotten the other thing.
This addresses some fuzz to make sure the comments don't lie. Notably, this defers contexts the same way between fake and real. This also pulls out handleShutdown to accent notes from #372 Signed-off-by: Adrian Cole <adrian@tetrate.io>
This addresses some fuzz to make sure the comments don't lie. Notably, this defers contexts the same way between fake and real. This also pulls out handleShutdown to accent notes from #372 Signed-off-by: Adrian Cole <adrian@tetrate.io> Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>
This is a follow-up for #371 (review) to test a set of possible signals (
SIGINT,SIGTERM,SIGKILL) to terminate func-e.A minor change on checking the stderr: this PR uses a scanner instead of peeking on the buffer since for some reason redirecting
cmd.Stderrto a buffer doesn't give complete stderr after the process is terminated. Hence here we use a scanner toio.ReadCloserreturned bycmd.StderrPipe()instead.Signed-off-by: Dhi Aurrahman dio@rockybars.com