pulumi / providertest

Incubating facilities for testing Pulumi providers
Apache License 2.0
5 stars 0 forks source link

Try to not assume testing.T #79

Closed t0yv0 closed 5 months ago

t0yv0 commented 5 months ago

Generalizing from concrete testing.T allows using in other test contexts, such as rapid testing.

Breaking: remove PulumiTest.T() accessor:

func (a *PulumiTest) T() T
codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 60.00000% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 37.29%. Comparing base (3a38438) to head (41d2193). Report is 1 commits behind head on main.

:exclamation: Current head 41d2193 differs from pull request most recent head 1627f30. Consider uploading reports for the commit 1627f30 to get more accurate results

Files Patch % Lines
pulumitest/pulumiTest.go 42.85% 4 Missing :warning:
pulumitest/run.go 55.55% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #79 +/- ## ========================================== - Coverage 37.35% 37.29% -0.06% ========================================== Files 43 43 Lines 2677 2681 +4 ========================================== Hits 1000 1000 - Misses 1569 1573 +4 Partials 108 108 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

t0yv0 commented 5 months ago

Yeah I was trying to build an interface that satisfies the constraints in Rapid, which uses its' own TB definition https://pkg.go.dev/pgregory.net/rapid#TB - I can see if the generic one is compatible .

t0yv0 commented 5 months ago

Soo. testing.TB is not quite sufficient as it's missing Deadline that this code uses. Something like this compiles:

type T interface {
    testing.TB
    Deadline() (time.Time, bool)
}

But testing.TB imposes more constraints like Setenv(key, value string) which this framework does not use and rapid does not implement (indeed they're sketchy there). If we go this direction, using this library with rapid will require more shims. I think requiring methods we don't need hurts composability a bit so I'd stick with minimally viable interface.. Go likes minimal interfaces I understand.

t0yv0 commented 5 months ago

Curios on the use of testing.T(). This is mostly used internally to log messages. So returning nil or casting down *testing.T is not a good idea in the context where something else is used. I think I'm for taking the minor breaking change here and accepting as-is.

pulumitest/pulumiTest.go
74:func (a *PulumiTest) T() T {
previewProviderUpgrade.go
18: pulumiTest.T().Helper()
28:         test.T().Helper()
32:         test.T().Logf("writing grpc log to %s", grpcLogPath)
pulumitest/newStack.go
82: pt.T().Logf("creating stack %s", stackName)
pulumitest/run.go
19: pulumiTest.T().Helper()
31:         pulumiTest.T().Fatalf("failed to read stack export: %v", err)
34:         pulumiTest.T().Logf("run cache found at %s", options.CachePath)
36:         pulumiTest.T().Logf("no run cache found at %s", options.CachePath)
45:         isolatedTest.T().Logf("writing stack state to %s", options.CachePath)
48:             isolatedTest.T().Fatalf("failed to write snapshot to %s: %v", options.CachePath, err)
58:     pulumiTest.T().Fatalf("failed to fixup stack name: %v", err)
61:     pulumiTest.T().Logf("updating snapshot with fixed stack name: %s", stackName)
64:         pulumiTest.T().Fatalf("failed to write snapshot to %s: %v", options.CachePath, err)
danielrbradley commented 5 months ago

Perhaps if we're taking a breaking change is should include removing exposing of the testing.T entirely. Make it optional to pass in to allow logging and marking methods as helpers, but make things work without it better too - perhaps a deeper decoupling.

t0yv0 commented 5 months ago

Well I can rewrite internal references to tInternal()

t0yv0 commented 5 months ago

How about this version?

danielrbradley commented 5 months ago

@t0yv0 Here's a follow-up to take this a little further to:

  1. Treat "T" as a purely private concern
  2. Make the interface simpler to mock

https://github.com/pulumi/providertest/pull/80

t0yv0 commented 5 months ago

Generalizing from concrete *testing.T allows using pulumitest in other test contexts, such as rapid testing. A new PT interface is added to abstract over concrete testing contexts.

Breaking: remove PulumiTest.T() accessor:

func (a *PulumiTest) T() T