Skip to content

Conversation

@danielrbradley
Copy link
Contributor

@danielrbradley danielrbradley commented Aug 21, 2024

⚠️ Breaking change ⚠️

Don't capture T within the PulumiTest structure as this breaks various aspects of how Go's testing is designed:

  1. Sub-tests are given their own T instance so if we create a copy of a pulumitest instance for a sub-test, we don't want logs reported to the parent.
  2. Logs are incorrectly reported back to the wrong line number. The logs are tracked back to the construction of the pulumitest rather than the line of the operation that failed.

Fixes #104

Why pass in T?

  1. Logging: We can automatically log significant context in case of failure.
  2. Clean-up: When creating directories and stacks, we automatically register them for deletion after the test has passed.
  3. Assertions: We automatically assert that operations are successful.

Solution

  • Pass T as the first parameter for any method which does asserts internally as is the pattern in Go.
  • Write an automated migration to update all existing usages of the library.

Migration Script

We can fix pretty much all usages using gofmt.

gofmt -r 'a.Convert(b) -> a.Convert(t, b)' -w ./**/*_test.go
gofmt -r 'a.CopyTo(b) -> a.CopyTo(t, b)' -w ./**/*_test.go
gofmt -r 'a.CopyToTempDir() -> a.CopyToTempDir(t)' -w ./**/*_test.go
gofmt -r 'a.Destroy() -> a.Destroy(t)' -w ./**/*_test.go
gofmt -r 'a.ExportStack() -> a.ExportStack(t)' -w ./**/*_test.go
gofmt -r 'a.GrpcLog() -> a.GrpcLog(t)' -w ./**/*_test.go
gofmt -r 'a.ClearGrpcLog() -> a.ClearGrpcLog(t)' -w ./**/*_test.go
gofmt -r 'a.Import(b, c, d, e, f, g) -> a.Import(t, b, c, d, e, f, g)' -w ./**/*_test.go
gofmt -r 'a.Import(b, c, d, e, f) -> a.Import(t, b, c, d, e, f)' -w ./**/*_test.go
gofmt -r 'a.Import(b, c, d, e) -> a.Import(t, b, c, d, e)' -w ./**/*_test.go
gofmt -r 'a.Import(b, c, d) -> a.Import(t, b, c, d)' -w ./**/*_test.go
gofmt -r 'a.Import(b, c) -> a.Import(t, b, c)' -w ./**/*_test.go
gofmt -r 'a.Import(b) -> a.Import(t, b)' -w ./**/*_test.go
gofmt -r 'a.ImportStack(b) -> a.ImportStack(t, b)' -w ./**/*_test.go
gofmt -r 'a.Install() -> a.Install(t)' -w ./**/*_test.go
gofmt -r 'a.InstallStack(b) -> a.InstallStack(t, b)' -w ./**/*_test.go
gofmt -r 'a.Preview() -> a.Preview(t)' -w ./**/*_test.go
gofmt -r 'a.Refresh() -> a.Refresh(t)' -w ./**/*_test.go
gofmt -r 'a.SetConfig(b, c) -> a.SetConfig(t, b, c)' -w ./**/*_test.go
gofmt -r 'a.Up() -> a.Up(t)' -w ./**/*_test.go
gofmt -r 'a.UpdateSource(b) -> a.UpdateSource(t, b)' -w ./**/*_test.go
gofmt -r 'a.NewStack(b) -> a.NewStack(t, b)' -w ./**/*_test.go
gofmt -r 'a.Run(b) -> a.Run(t, b)' -w ./**/*_test.go
  • It's a little messy but correctly migrates all usages within this package.
  • gofmt seems unable to migrate the pt.Run() because it can't match inline functions. Re-testing shows this works.
  • The Import migration isn't idempotent due to the variable number of arguments so should be run once manually when upgrading to the new version and the changes committed.
  • This doesn't handle any instances of passing additional functional argument, but I doubt we use many of these in practice.

@danielrbradley danielrbradley self-assigned this Aug 21, 2024
@danielrbradley danielrbradley marked this pull request as draft August 21, 2024 10:02
- Remove `t` from the PulumiTest struct.
- Fix calls between methods.
Run the following migration:

```
gofmt -r 'a.Convert(b) -> a.Convert(t, b)' -w ./**/*.go
gofmt -r 'a.a.CopyTo(b) -> a.a.CopyTo(t, b)' -w ./**/*.go
gofmt -r 'a.CopyToTempDir() -> a.CopyToTempDir(t)' -w ./**/*.go
gofmt -r 'a.Destroy() -> a.Destroy(t)' -w ./**/*.go
gofmt -r 'a.ExportStack() -> a.ExportStack(t)' -w ./**/*.go
gofmt -r 'a.GrpcLog() -> a.GrpcLog(t)' -w ./**/*.go
gofmt -r 'a.ClearGrpcLog() -> a.ClearGrpcLog(t)' -w ./**/*.go
gofmt -r 'a.Import(b, c, d, e, f, g) -> a.Import(t, b, c, d, e, f, g)' -w ./**/*.go
gofmt -r 'a.Import(b, c, d, e, f) -> a.Import(t, b, c, d, e, f)' -w ./**/*.go
gofmt -r 'a.Import(b, c, d, e) -> a.Import(t, b, c, d, e)' -w ./**/*.go
gofmt -r 'a.Import(b, c, d) -> a.Import(t, b, c, d)' -w ./**/*.go
gofmt -r 'a.Import(b, c) -> a.Import(t, b, c)' -w ./**/*.go
gofmt -r 'a.Import(b) -> a.Import(t, b)' -w ./**/*.go
gofmt -r 'a.ImportStack(b) -> a.ImportStack(t, b)' -w ./**/*.go
gofmt -r 'a.Install() -> a.Install(t)' -w ./**/*.go
gofmt -r 'a.InstallStack(b) -> a.InstallStack(t, b)' -w ./**/*.go
gofmt -r 'a.Preview() -> a.Preview(t)' -w ./**/*.go
gofmt -r 'a.Refresh() -> a.Refresh(t)' -w ./**/*.go
gofmt -r 'a.SetConfig(b, c) -> a.SetConfig(t, b, c)' -w ./**/*.go
gofmt -r 'a.Up() -> a.Up(t)' -w ./**/*.go
gofmt -r 'a.UpdateSource(b) -> a.UpdateSource(t, b)' -w ./**/*.go
gofmt -r 'a.NewStack(b) -> a.NewStack(t, b)' -w ./**/*.go
gofmt -r 'a.Run(b) -> a.Run(t, b)' -w ./**/*.go
```

The only manual tweak needed is for pt.Run where 1 additional option is passed. We can't have a rule for 'a.Run(b, c) -> a.Run(t, b, c)' because it would also affect all uses of sub-tests. The pt.Run function is not widely used directly as it's a helper designed for upgrade testing.
@danielrbradley danielrbradley marked this pull request as ready for review August 22, 2024 09:05
@danielrbradley danielrbradley requested a review from a team August 22, 2024 09:09
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unfortunate change, but I think a positive one. LGTM

Copy link
Contributor

@flostadler flostadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a useful change! LGTM

I love that you created a migration script!

@danielrbradley danielrbradley merged commit be10035 into main Aug 22, 2024
@danielrbradley danielrbradley deleted the pass-t branch August 22, 2024 12:37
@pulumi-bot
Copy link

This PR has been shipped in release v0.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop capturing testing.T instance

4 participants