Provide a useful implementation of something compatible with testing.T

mrsheepuk commented 9 months ago

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

:zap: New feature (non-breaking change which adds new behaviour)

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

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

vearutop commented 9 months ago

codecov[bot] commented 9 months ago

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

```diff @@ 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.

mrsheepuk commented 9 months ago

This is related to #535.

Indeed - and #100. I'll sort out the test coverage etc if this looks like a reasonable approach?

vearutop commented 9 months ago

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.

mrsheepuk commented 9 months ago

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!)

mrsheepuk commented 9 months ago

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?

mrsheepuk commented 8 months ago

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 👍

mrsheepuk commented 4 months ago

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.

b0ch3nski commented 4 weeks ago

@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:

mirogta commented 4 weeks ago

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

mrsheepuk commented 3 weeks ago

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

b0ch3nski commented 3 weeks ago

@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 commented 3 weeks ago

@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:

Oh, you're not wrong! it's idiomatic go testing vs idiomatic godog usage. For us, the latter ended up being more pragmatic.

mrsheepuk commented 3 weeks ago

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.

mrsheepuk commented 3 weeks ago

@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.

mrsheepuk commented 3 weeks ago

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

vearutop commented 3 weeks ago

hello folks, I will look into this on Friday (once I'm back from vacation 😅)

vearutop commented 3 weeks ago

and thanks for keeping this alive!

mrsheepuk commented 3 weeks ago

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 👍

mrsheepuk commented 2 weeks ago

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 🚀

cucumber / godog