cucumber / godog

Cucumber for golang
MIT License
2.21k stars 249 forks source link

Provide access to the testing.T log function in the context #570

Closed mrsheepuk closed 9 months ago

mrsheepuk commented 9 months ago

πŸ€” What's changed?

Simply adding an interface over testing.T's log and logf functions to the context, allowing logging of information contextually. Vaguely related to #535 and #100 as, if this pattern is OK, it could be expanded to provide the whole of testing.T.

Example usage:

func log(ctx context.Context, msg string, args ...interface{}) {
    if t := godog.GetTestLogger(ctx); t != nil {
        t.Logf(msg, args...)
    } else {
        fmt.Println(fmt.Printf(msg, args...))
    }
}

⚑️ What's your motivation?

We have longer-running asynchronous tests which, when run, we want to provide contextual feedback to the user on during the execution. By using the sub-test's testing.T, these log messages appear in a sane place in the formatted output.

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

Opening this early for feedback - if it is conceptually sane, I'll work this PR up to a better state and add docs - didn't want to bother documenting it in detail if it's not going to be acceptable tho!

πŸ“‹ Checklist:

tigh-latte commented 9 months ago

I like this idea, but, the use of if and else to change between loggers makes me think we could improve the API.

Given that log/slog now has a few functions which take context.Context as their first param, perhaps we could follow suit and expose a few log functions in godog. So your example would be something like:

func log(ctx context.Context, msg string, args ...any) {
    godog.Logf(ctx, msg, args...)
}

Under the hood, that/(those?) functions would check if we have a logger in context.Context, and act accordingly:

func Logf(ctx context.Context, msg string, args ...any) {
    if logger := getLogger(ctx); logger != nil {
        logger.Logf(msg, args...)
    } else {
        fmt.Println(msg+"\n", args...)
    }
}

godog would then expose every function in testing.T that we are comfortable with exposing.

mrsheepuk commented 9 months ago

Ooooh, yes, I see what you mean @tigh-latte - I'll have a further play and see if I can come up with a pattern that also solves the testify/assert problem (as I also hit that) without breaking the fundamental way of working...

mrsheepuk commented 9 months ago

Superseded by #571 - @tigh-latte if you could take a look there and see if that's close to what you had in mind πŸ˜„