godog
godog copied to clipboard
cucumber
Attempting to provide a useful implementation of something compatible with testing.T
🤔 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.
Still needs some work to tidy up, but sharing early for feedback before committing effort to adding better testing etc over it.
⚡️ 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, I believe. I'm testing this out at the moment in our project.
🏷️ What kind of change is this?
- :zap: New feature (non-breaking change which adds new behaviour)
♻️ Anything particular you want feedback on?
Again, raising early for feedback before committing too much time to making this mergeable and adding tests etc - happy to do that if the pattern is valid though - let me know 👍
📋 Checklist:
- [x] I agree to respect and uphold the Cucumber Community Code of Conduct
- [x] I've changed the behaviour of the code
- [ ] I have added/updated tests to cover my changes.
- [x] 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.
This text was originally generated from a template, then edited by hand. You can modify the template here.
Codecov Report
Attention: Patch coverage is 27.53623% with 50 lines in your changes are missing coverage. Please review.
Project coverage is 81.88%. Comparing base (
153db4e) to head (774cad0). Report is 4 commits behind head on main.
:exclamation: Current head 774cad0 differs from pull request most recent head 939bd89. Consider uploading reports for the commit 939bd89 to get more accurate results
| Files | Patch % | Lines |
|---|---|---|
| testingt.go | 21.42% | 32 Missing and 1 partial :warning: |
| suite.go | 37.03% | 17 Missing :warning: |
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
This is related to #535.
Indeed - and #100. I'll sort out the test coverage etc if this looks like a reasonable approach?
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 *testing.T is
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 assert.Equal, e.g.
// 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 os.Std* to capture *testing.T messages and turn them into step error.
But now I think a more stable solution could be to actually wrap *testing.T methods to collect errors first hand.
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 *testing.T, for example in case when godog is running as a standalone CLI.
To accommodate the case when user needs direct access to *testing.T (though I don't have an example why would that be useful) we can extend TestingT interface with an accessor:
TestingT() *testing.T
that would return value or nil depending on *testing.T availability.
Some methods don't seem to have clear semantics in scope of godog step, for example Run(name string, f func(t *testing.T)) bool or Parallel(), maybe we should exclude them from our interface.
Desired behavior of Log(args ...any) is also not very clear to me, as godog can use different formatters and perhaps some people may want to see those logged messages not as console output, but as a part of the file report.
Hi @vearutop thanks for taking the time to look at this!
Previously, I was thinking we could hack around os.Std* to capture *testing.T messages and turn them into step error. But now I think a more stable solution could be to actually wrap *testing.T methods to collect errors first hand.
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:
https://github.com/cucumber/godog/pull/571/files#diff-8a638a6bfeccc0ce1df4924327f499d69feb7bf391f82a4ba0d22312cfafc4faR112-R116
Combined with this:
https://github.com/cucumber/godog/pull/571/files#diff-a883af0d9a825770682af12c8ccc9bfaef5da1db285bfe066aafb50da3e5f87aR54-R82
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:
func ItShouldHavePropertyPopulated(ctx context.Context, property string) error {
bv, err := getProp(ctx, property)
if err != nil {
return err
}
assert.NotEmpty(godog.GetTestingT(ctx), bv.String())
return nil
}
func ItShouldHavePropertyOfJSONValue(ctx context.Context, property string, value string) error {
bv, err := getProp(ctx, property)
if err != nil {
return err
}
value = ReplaceUnique(ctx, value)
assert.JSONEq(godog.GetTestingT(ctx), value, bv.String())
return nil
}
... and the resulting test step is failing / passing as expected when the properties are empty and/or the JSON is matched/mismatched, e.g.:
And it should have 'status.capacity' of JSON [{"stage":"nonprod", "hasCapacities":true},{"stage":"prod", "hasCapacity":true}] # sharedsteps.go:206 -> github.com/appvia/wayfinder/integration/api.ItShouldHavePropertyOfJSONValue
Error Trace: ./integration/api/sharedsteps.go:214
/usr/local/Cellar/go/1.20.3/libexec/src/reflect/value.go:586
/usr/local/Cellar/go/1.20.3/libexec/src/reflect/value.go:370
./vendor/github.com/cucumber/godog/internal/models/stepdef.go:182
./vendor/github.com/cucumber/godog/suite.go:220
./vendor/github.com/cucumber/godog/suite.go:485
./vendor/github.com/cucumber/godog/suite.go:539
Error: Not equal:
expected: []interface {}{map[string]interface {}{"hasCapacities":true, "stage":"nonprod"}, map[string]interface {}{"hasCapacity":true, "stage":"prod"}}
actual : []interface {}{map[string]interface {}{"hasCapacity":true, "stage":"nonprod"}, map[string]interface {}{"hasCapacity":true, "stage":"prod"}}
Diff:
--- Expected
+++ Actual
@@ -2,3 +2,3 @@
(map[string]interface {}) (len=2) {
- (string) (len=13) "hasCapacities": (bool) true,
+ (string) (len=11) "hasCapacity": (bool) true,
(string) (len=5) "stage": (string) (len=7) "nonprod"
then the overall scenario fails with the message too:
--- Failed steps:
Scenario: Add AWS cluster network plan with reference to assignable networks # features/clusternetworks/cluster-network-plans.feature:18
And it should have 'status.capacity' of JSON [{"stage":"nonprod", "hasCapacities":true},{"stage":"prod", "hasCapacity":true}] # features/clusternetworks/cluster-network-plans.feature:31
Error:
Error Trace: ./integration/api/sharedsteps.go:214
/usr/local/Cellar/go/1.20.3/libexec/src/reflect/value.go:586
/usr/local/Cellar/go/1.20.3/libexec/src/reflect/value.go:370
./vendor/github.com/cucumber/godog/internal/models/stepdef.go:182
./vendor/github.com/cucumber/godog/suite.go:220
./vendor/github.com/cucumber/godog/suite.go:485
./vendor/github.com/cucumber/godog/suite.go:539
Error: Not equal:
expected: []interface {}{map[string]interface {}{"hasCapacities":true, "stage":"nonprod"}, map[string]interface {}{"hasCapacity":true, "stage":"prod"}}
actual : []interface {}{map[string]interface {}{"hasCapacity":true, "stage":"nonprod"}, map[string]interface {}{"hasCapacity":true, "stage":"prod"}}
Diff:
--- Expected
+++ Actual
@@ -2,3 +2,3 @@
(map[string]interface {}) (len=2) {
- (string) (len=13) "hasCapacities": (bool) true,
+ (string) (len=11) "hasCapacity": (bool) true,
(string) (len=5) "stage": (string) (len=7) "nonprod"
(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!)
Desired behavior of Log(args ...any) is also not very clear to me, as godog can use different formatters and perhaps some people may want to see those logged messages not as console output, but as a part of the file report.
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 testing.T :crying_cat_face:
If it helps, I've used a wrapper around testing.T in order to use testify's require/assert. See https://github.com/mirogta/godog-assert and a (stale) discussion here
@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 testify already does just to return errors is fighting against an idiomatic way of doing things :thinking:
@mrsheepuk I have kind of opposite feeling that reimplementing everything that
testifyalready does just to return errors is fighting against an idiomatic way of doing things :thinking:
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.
@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 😅)
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 👍
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 🚀