-
Notifications
You must be signed in to change notification settings - Fork 249
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 a useful implementation of something compatible with testing.T #571
Conversation
This is related to #535. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #571 +/- ##
==========================================
- Coverage 83.21% 81.88% -1.33%
==========================================
Files 28 29 +1
Lines 3413 3478 +65
==========================================
+ Hits 2840 2848 +8
- Misses 458 516 +58
+ Partials 115 114 -1 ☔ View full report in Codecov by Sentry. |
I like the idea of type TestingT interface {
Log(args ...interface{})
Logf(format string, args ...interface{})
Errorf(format string, args ...interface{})
Fail()
FailNow()
} returned from the context. The full public interface of type TestingT interface {
Parallel()
Setenv(key, value string)
Run(name string, f func(t *testing.T)) bool
Deadline() (deadline time.Time, ok bool)
Name() string
Fail()
Failed() bool
FailNow()
Log(args ...any)
Logf(format string, args ...any)
Error(args ...any)
Errorf(format string, args ...any)
Fatal(args ...any)
Fatalf(format string, args ...any)
Skip(args ...any)
Skipf(format string, args ...any)
SkipNow()
Skipped() bool
Helper()
Cleanup(f func())
TempDir() string
} Maybe we should try to provide as much as possible with ours. But I'm still not very clear about the use cases how such instance would be used and what would be behavior expectations. For example, I can imagine a step definition that would use // iShouldHaveValue is a step definition.
func iShouldHaveValue(ctx context.Context, value string) {
t := godog.T(ctx) // Getting a TestingT from context.
assert.Equal(t, "someExpectedValue", value)
} and then we would need to capture assertion error if any and use it somewhat similar to // iShouldHaveValue is a step definition.
func iShouldHaveValue(ctx context.Context, value string) error {
if value != "someExpectedValue" {
return errors.New("unexpected value")
}
return nil
} Previously, I was thinking we could hack around e.g. something like func (dt *dogTestingT) Errorf(format string, args ...interface{}) {
df.errors = append(df.errors, fmt.Errorf(format, args...)) // df.errors is later checked by step executor. This could be implemented even in absense of actual To accommodate the case when user needs direct access to TestingT() *testing.T that would return value or nil depending on Some methods don't seem to have clear semantics in scope of godog step, for example Desired behavior of |
Hi @vearutop thanks for taking the time to look at this!
I think you've come to the same conclusion of what I was aiming for here - that's pretty much how this PR works - see this bit of magic here: Combined with this: With that, you can use it exactly as you suggest - from my (unfortunately currently private) repo where I'm using this fork, I'm doing things like this:
... and the resulting test step is failing / passing as expected when the properties are empty and/or the JSON is matched/mismatched, e.g.:
then the overall scenario fails with the message too:
(all the 'Error Trace' stuff is just what testify's assert.* functions output to testing.T, I don't particularly like all that noise in the test output but that's a different problem!) |
This is a very fair point - my use case was just seeing the output on the CI log while these tests were running, but it could/should perhaps be more nuanced. Given that there is no custom logging support at all now, perhaps this could be handled in a separate PR to make the Log functions more nuanced in terms of recording the output? |
Just a quick update on this one - we're using it quite effectively ourselves so I'll tidy this up (hopefully early next week) with some tests and examples to make it clearer what the intended usage is then we can go from there 👍 |
I know I never came back to this one - in the end, we ended up sticking with more idiomatic godog 'return an error' directly instead of using testify's require/asset libraries in our use case, so we're only actually using the Log() part of what's on this PR. I don't know if this is worth keeping alive - I probably won't have time to add much more to it in the way of tests etc and I'm not certain that it's idiomatically sensible to encourage people down this route any more... it does work, I'm just less-than-convinced it is correct (beyond the log bit, which is useful, but could be skinned a different way). Any thoughts? If y'all feel it is generally useful, I'll find the time to tidy it up and get it into a mergeable state, but if not, I'm happy to close it unmerged. |
@mrsheepuk What needs to be done here to ship this? We are stuck using (not much maintained these days) https://github.com/go-bdd/gobdd instead of Godog just because of missing |
If it helps, I've used a wrapper around |
@b0ch3nski what is here does work, we're using it pretty heavily - but mostly just for the contextual logging in the output, we moved away from using testify as it felt like we were fighting the cucumber/godog idiomatic way of doing things, so we simply embraced that and returned errors. Regarding getting this merged - I'm not really sure - @vearutop if I add some tests and bring this up to date, are you conceptually happy with this going in? @mirogta that looks like a nice clean narrow approach from a quick look - might be worth seeing if that could be a cleaner approach to a PR here? |
@mrsheepuk I have kind of opposite feeling that reimplementing everything that |
Oh, you're not wrong! it's idiomatic go testing vs idiomatic godog usage. For us, the latter ended up being more pragmatic. |
Just an update - I'm going ahead and adding some test coverage over this and tidying it up to implement as much as makes sense of testing.T's public interface, so should have something mergeable shortly. |
@b0ch3nski - would you be interested in trying to use this branch to validate if it meets your needs? I've added some tests and tidied up the code a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrsheepuk Thank you very much for this 🦾 I'd be more than happy to see this merged upstream. One nitpick suggestion added to enforce TestingT
interface compatibility but yeah might be too paranoid 😄
Co-authored-by: Piotr Bocheński <bochenski.piotr@gmail.com>
@b0ch3nski I've updated the godog-assert example and renamed 'GetTestingT(ctx)' to 'T(ctx)'. @vearutop are you someone who can cast an eye over this from a maintainer perspective? or is there someone better who can review? |
hello folks, I will look into this on Friday (once I'm back from vacation 😅) |
and thanks for keeping this alive! |
hi @vearutop - just a quick update, I improved the test coverage and updated the README and CHANGELOG so I think from my perspective, this is ready to go assuming you're happy. of course, happy to make any further changes you think needed after reviewing, so let me know what you think 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you for this upgrade.
I've commented a couple of minor things, and then I think this is good to merge. 👍
Thanks for the review @vearutop - updated and ready for another try at CI 🚀 |
🤔 What's changed?
Supersedes #570 building on the comment there and providing an implementation of a testing.T-like interface suitable for use with testify's assert and require libraries.
This provides a much cleaner implementation for providing compatibility with libraries like testify's assert and require. The godog-assert example has been updated to show the new cleaner usage.
⚡️ What's your motivation?
Started with trying to give contextual access to the step's testing.T Logf (see #570 for the first attempt at that) but with @tigh-latte 's comment there, I thought it worth expanding it to allow testify's assert and require libraries to work sanely with a testing.T-like interface.
They should work irrespective of whether a testing.T is registered in the test.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
We've been using this internally now for a while and found it to work well, so pretty much, happy for this to go in. Let me know if there's anything more that needs doing on it to get it merged.
📋 Checklist: