Skip to content

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

Closed
mrsheepuk wants to merge 1 commit intocucumber:mainfrom
mrsheepuk:add-t-to-context
Closed

Provide access to the testing.T log function in the context#570
mrsheepuk wants to merge 1 commit intocucumber:mainfrom
mrsheepuk:add-t-to-context

Conversation

@mrsheepuk
Copy link
Copy Markdown
Contributor

@mrsheepuk mrsheepuk commented Aug 14, 2023

🤔 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
Copy Markdown
Member

tigh-latte commented Aug 16, 2023 •
edited
Loading

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

mrsheepuk commented Aug 17, 2023

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 mrsheepuk mentioned this pull request Aug 18, 2023
Merged
7 tasks
@mrsheepuk
Copy link
Copy Markdown
Contributor Author

mrsheepuk commented Aug 18, 2023

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mrsheepuk @tigh-latte