Better timeout handling

[marcinwyszynski]

No description provided.

[marcosnils]

I'm not sure what this is actually improving. IIUC this only adds the ability to set a default timeout via env variables?

[marcosnils]

This function is never called / used?

[marcinwyszynski]

Correct, this is user-facing API.

[marcosnils]

Ok, in that case this needs to be tested. However don't add anything until we decide if it makes sense to add this functionality or not. I still don't quite understand what this is improving / fixing.

[marcinwyszynski]

The issue I'm trying to address here is that if you have different tests written using different frameworks (eg. we have a bit of testify) then passing a CLI flag (goblin.timeout) is not a viable option.

Re: SetDefaultTimeout - this gives you the ability to set per test (rather than test-case) timeout rather than using the global flag. Hence the indirection, plus the ability to hijack the intermediate step (defaultTimeout).

Hope that makes sense?

[marcosnils]

then passing a CLI flag (goblin.timeout) is not a viable option.

Interesting.. is go test complaining if you pass that arugment?

Hope that makes sense?

Kind of ... there's something that I'm not seeing here. You basically added a new defaultTimeout vs currentTimeout which are a bit hard for me to reason about.

We basically have:

• goblin.timeout arg
• GOBLIN_TIMEOUT env var
• SetDefaultTimeout function

Besides this PR needing docs and tests, it's hard to have a mental model about what happens if I set one or many of these variables.

[marcinwyszynski]

Interesting.. is go test complaining if you pass that arugment?

Correct:

hard for me to reason about

Yeah, I see what you mean. TBH the main thing I care about is the ability to set the global timeout using an environment variable. Would you be open to accepting a PR with just that, obviously including tests?

[marcosnils]

TBH the main thing I care about is the ability to set the global timeout using an environment variable. Would you be open to accepting a PR with just that, obviously including tests?

yes, that's ok. Out of curiosity, do you have a simple example to reproduce this case you have where you need goblin but sending the argument doesn't cut it? Seems like you ran into a particular case that might have a proper fix instead of adding a new feature.

[marcinwyszynski]

@marcosnils The problem is with using the flag package, which barfs at anything it does not recognize (default strategy for flag.Parse being ExitOnError). Not a great idea if different libraries need different flags. Anyway, even if Goblin does not use this package, stretchr/testify/suite does. So there isn't really a good fix on Goblin's end, though removing the dependency on the flag package would make it a better citizen of the ecosystem.

[marcosnils]

In that case then maybe we can make Globin have it's own flagset through https://pkg.go.dev/flag#NewFlagSet and set the error handling to be ContinueOnError instead of adding the env variable? WDYT?

[marcinwyszynski]

Sure, good idea in general, but that still does not make it possible to use Goblin with flags with other frameworks using the flag package. In principle, using custom flags isn't particularly good design.

[marcosnils]

, but that still does not make it possible to use Goblin with flags with other frameworks using the flag package.

agree here

In principle, using custom flags isn't particularly good design.

I don't agree here. It really depends on the use-case. In any case, I don't want this to become a debate a of when and how to use flags. Adding the support to the env variable SGTM so we can support this use-case

[marcinwyszynski]

OK let's not get into philosophical debates. I'll rewrite this PR into something that only covers the envvar, plus tests.