Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

mrsheepuk
Copy link
Contributor

🤔 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?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ 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:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

@tigh-latte
Copy link
Member

tigh-latte commented Aug 16, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mrsheepuk mrsheepuk closed this Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants