Test output expectations should fail if the process has prematurely exited. #12
Conversation
|
|
helpers.go
Outdated
| // This can be used within a select{} statement. | ||
| // If waitExtra is given, waits a little bit before sending cmdExit info. This allows any fellow | ||
| // switch cases to handle unprocessed stdout. | ||
| func ttExited(tt *TermTest, waitExtra bool) chan *cmdExit { |
There was a problem hiding this comment.
| func ttExited(tt *TermTest, waitExtra bool) chan *cmdExit { | |
| func (tt *TermTest) waitForCmdExit(waitExtra bool) chan *cmdExit { |
Note as this is a property of termtest we should move this out of helpers.
helpers.go
Outdated
| select { | ||
| case <-ticker.C: | ||
| if tt.exited != nil { | ||
| if waitExtra { |
There was a problem hiding this comment.
Could you explain the rationale for this in a comment?
There was a problem hiding this comment.
It is explained in the function documentation. Should I move/copy that comment here?
There was a problem hiding this comment.
No I understand what the param does. What I'm asking is why is this a thing at all? I see this function is called from two different places, once with this param set to true and once set to false. How did you decide one of them needed this and the other didn't?
There was a problem hiding this comment.
I added some more comments around this. Basically, you want to wait if you have sibling output consumers that are expecting output so that you don't immediately fail the expect if the process exits. You don't want to wait if you are expecting the process to exit or have exited (i.e. cp.ExpectExit*()). As a side note, if we were to always wait, then every cp.ExpectExit*() would take slightly longer.
outputconsumer.go
Outdated
| e.mutex.Lock() | ||
| e.opts.Logger.Println("Encountered timeout") | ||
| return fmt.Errorf("after %s: %w", e.opts.Timeout, TimeoutError) | ||
| case state := <-ttExited(e.tt, true): |
There was a problem hiding this comment.
I guess this is an error condition, because normally the other selects would have fired before this one? Please add a comment explaining.
Though I wonder if this really should be here. Processes exiting non-zero is not an error condition to termtest. Termtest is meant to be used for testing processes in a terminal context, and failure conditions such as non-zero exits are part of that scope. Again though, I might be missing something, so if we want to keep this definitely add a comment.
There was a problem hiding this comment.
Not quite. The first case would have fired if there is an output match. The second case would have fired if an ultimate timeout was reached, and the process was still active. The third case (this one) will fire if the process exits (either normally, or abnormally; it does not matter) without either of the two cases having (yet) fired.
The most common case of this firing is if we typo an expectation and the process exits normally with unmatched output. I assume this is exactly the case we're trying to catch with this ticket.
There was a problem hiding this comment.
Nice, so this would basically alert you that you forgot a tt.ExpectExit() of some sort? That's a nice win!
There was a problem hiding this comment.
In some cases, yes, I suppose so.
|
I'm flagging for re-review just to get feedback on my replies. I'll make the requested changes tomorrow and then submit for another review. Thanks for your understanding. |
helpers.go
Outdated
| select { | ||
| case <-ticker.C: | ||
| if tt.exited != nil { | ||
| if waitExtra { |
There was a problem hiding this comment.
No I understand what the param does. What I'm asking is why is this a thing at all? I see this function is called from two different places, once with this param set to true and once set to false. How did you decide one of them needed this and the other didn't?
outputconsumer.go
Outdated
| e.mutex.Lock() | ||
| e.opts.Logger.Println("Encountered timeout") | ||
| return fmt.Errorf("after %s: %w", e.opts.Timeout, TimeoutError) | ||
| case state := <-ttExited(e.tt, true): |
There was a problem hiding this comment.
Nice, so this would basically alert you that you forgot a tt.ExpectExit() of some sort? That's a nice win!
Naatan
left a comment
Naatan
left a comment
There was a problem hiding this comment.
Looks good, thanks for the comment, makes sense!
The idea is to call
exec.Cmd.Wait()in a goroutine (https://github.com/ActiveState/termtest/pull/12/files#diff-13ff3d9e77d1ebdf70d066552cb9e504472550dc97fd74aac53a8ab3a63e54a1) and edit the Expect() functions (https://github.com/ActiveState/termtest/pull/12/files#diff-a63ecf8315eeb0943ebec69499243bd650ae6b99635bde7aa1c0f6ac773368c2R106-R111 and https://github.com/ActiveState/termtest/pull/12/files#diff-2a74ac0f372490e4a5cf19a11e38b9329713643dc44c65ca5f7de285f84c9391R183-R187) to poll for a command's exit status alongside the usual expect checks. A helper function that returns a channel is used to facilitate use insideselect{}statements (https://github.com/ActiveState/termtest/pull/12/files#diff-296a01a801f363652ec765418086e861f5db0287df573d54c8fde4eebe0e26e9R43).ActiveState/cli#3309 uses this PR's commit when running integration tests for State Tool to verify that it doesn't break anything at least.