ensure withTimeout deals nicely with runtime.Goexit()

[rubensayshi]

We've been running our tests from an OnStart hook, but when one of the tests calls t.FailNow() it will under-the-hood call runtime.Goexit() and then the OnStart hook just sits there waiting for the timeout...

With this change the withTimeout can never get stuck when for some reason runtime.Goexit() got called.

I'm not 100% sure returning the fmt.Errorf("withTimeout exit without returning") is the best choice,
c <- nil could also work, that would make it look like the callback succeeded and continue calling the next OnStart or OnStop hook.

[sywhang]

Hey @rubensayshi, thanks for submitting this! Left a suggestion for a fix here.

Re: the error message, I believe withTimeout exit without returning is a little too implementation-detail level error message for most users to comprehend.

Can we instead say something like: OnStart callback __ exited without returning? (and it'd be OnStop callback exited... for onstop callbacks).

I can also make those changes to this PR on your behalf if you want. Let me know

[rubensayshi]

updated to include both your suggestions, I don't think its strictly necessary as there isn't a race in the normal scenario, but with the boolean it might prevent odd future issues and it's more clear what the intention of the code is as well.

I was thinking ... I think the only scenario where this gets triggered is a runtime.Goexit() ... I'm not sure. but maybe the right response would be to runtime.Goexit() in withTimeout as well?
Though returning an error seems cleaner and clearer to the caller.

[rubensayshi]

updated to make the error get the wrapping with the "... hook added by ...", so an error looks like;

OnStart hook added by package/path.Func failed: exited without returning

[sywhang]

Thanks for making the changes. There are linter and test failures that I wanted to address, but because this is a fork that lives in an organization, I couldn't push these changes directly to this PR. If you merge prvbl#1 into this branch that should update this PR.

[rubensayshi]

hey @sywhang thanks for that, you could have also commented and I would have changed, but much appreciated that you submitted the suggested changes ;)

PR updated to include you changes.

[codecov]

Codecov Report

Merging #890 (5a41d4c) into master (e4e7928) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #890   +/-   ##
=======================================
  Coverage   98.53%   98.53%           
=======================================
  Files          30       30           
  Lines        1157     1163    +6     
=======================================
+ Hits         1140     1146    +6     
  Misses         11       11           
  Partials        6        6           

Impacted Files	Coverage Δ
app.go	95.04% <100.00%> (+0.13%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4e7928...5a41d4c. Read the comment docs.

[sywhang]

Thanks. This looks good to me now. Looks like we aren't able to merge yet because the branch is out of date with the base branch. If you can make the branch up to date with base branch, we'll go ahead and merge this.

[rubensayshi]

rebased, and thanks for helping me polish this <3

[sywhang]

Great! Merging now. Thanks again for your contribution.