Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3390 +/- ##
==========================================
- Coverage 70.05% 70.05% -0.01%
==========================================
Files 342 342
Lines 37030 37030
==========================================
- Hits 25943 25941 -2
- Misses 9336 9337 +1
- Partials 1751 1752 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: repro test for MaxItems=1 update panic
This is a well-scoped, intentionally-failing draft PR. The test faithfully mimics the real GCP provider shape (nested computed MaxItems=1 fields, ignore_changes guard, cloud readback returning multiple policies) and the failure mode is reproducible and clearly documented.
No blocking issues found. All observations are non-blocking and are called out inline:
docs/guides/testing.mdreferencespkg/tests/refresh_cross_test.go, which doesn't exist onmainor in this diff — likely needs updating or a note that the file is planned.pulCheckRecoveringReadPanicsinrefresh.gois missing theskipUnlessLinux(t)gate thatpulcheck.PulCheckprovides, creating asymmetric behaviour on non-Linux CI.- The outer
recoveringReadPanicsServerwrapper appears redundant becausePanicRecoveringProviderServer(returned byProviderServerFromInfo) already converts panics to gRPC errors before the outerdefer/recovercould fire. setPanicRecoveringProviderServerFieldis duplicated verbatim betweenrefresh.goandupdate_cross_test.go, and both useunsafe.Pointer+ reflection to write to private fields. A small testing-only constructor onPanicRecoveringProviderServerwould make this statically safe and avoid the duplication.
These are all appropriate to address in the fix PR rather than here. Approving so progress is not blocked on the draft.
Reviewed by Internal Trusted PR Reviewer
To install this agentic workflow, run
gh aw add pulumi-labs/gh-aw-internal/.github/workflows/gh-aw-pr-review.md@99a309bf18258dbae98c8176ea10657e0b08deca
| Examples worth consulting: | ||
|
|
||
| - Diff parity (`pkg/internal/tests/cross-tests/diff_cross_test.go`) | ||
| - Refresh regressions (`pkg/tests/refresh_cross_test.go`) |
There was a problem hiding this comment.
Non-blocking: This line references pkg/tests/refresh_cross_test.go, but that file does not exist on main and is not among the files added by this PR. The existing file on main is pkg/tests/refresh_test.go. If the cross-test file is planned for a follow-up PR, please note that here; otherwise, point to the existing file to avoid misleading contributors.
| } | ||
| } | ||
|
|
||
| func pulCheckRecoveringReadPanics( |
There was a problem hiding this comment.
Non-blocking: pulCheckRecoveringReadPanics is intended as a drop-in replacement for pulcheck.PulCheck when panic recovery is needed, but it is missing the skipUnlessLinux(t) call that pulcheck.PulCheck makes. On a non-Linux CI run, the normal Refresh path (when recoverReadPanics is false) will be skipped, but a call with RefreshRecoverReadPanics() will not be skipped, causing asymmetric test execution. Adding skipUnlessLinux(t) as the first statement (or a call to pulcheck.PulCheck with an option to customise the server) would close this gap.
|
|
||
| handle, err := rpcutil.ServeWithOptions(rpcutil.ServeOptions{ | ||
| Init: func(srv *grpc.Server) error { | ||
| pulumirpc.RegisterResourceProviderServer(srv, &recoveringReadPanicsServer{ |
There was a problem hiding this comment.
Non-blocking: pulcheck.ProviderServerFromInfo already returns a *providerserver.PanicRecoveringProviderServer, which wraps every method (including Read) with its own defer/recover. Registering &recoveringReadPanicsServer{ResourceProviderServer: prov} around it means a Read panic would be caught by the inner wrapper and turned into a gRPC error long before the outer defer/recover in recoveringReadPanicsServer.Read fires. The outer wrapper appears redundant. If there is a path where a panic can escape the inner server, that should be explained in a comment; otherwise the outer layer can be removed.
| setPanicRecoveringProviderServerField(t, wrapped, "omitStackTraces", true) | ||
| } | ||
|
|
||
| func setPanicRecoveringProviderServerField( |
There was a problem hiding this comment.
Non-blocking: setPanicRecoveringProviderServerField (and prepareProviderForRecoveredReadPanics) use unsafe.Pointer + reflect.NewAt to write to unexported fields of PanicRecoveringProviderServer. This creates a hidden compile-time-invisible coupling: renaming either logger or omitStackTraces would cause a silent runtime panic rather than a build failure. Consider surfacing a testing-only constructor or option on PanicRecoveringProviderServer (e.g., NewForTest(inner, logger, omitStackTraces)) to make this dependency explicit and statically checked. The same pattern is duplicated in pkg/tests/update_cross_test.go; centralising it would halve the surface area that needs updating when the struct changes.
| ) | ||
| } | ||
|
|
||
| func prepareProviderForBridgePanics(t *testing.T, server pulumirpc.ResourceProviderServer) { |
There was a problem hiding this comment.
Non-blocking: setPanicRecoveringProviderServerField and prepareProviderForBridgePanics here are nearly line-for-line copies of the same helpers in pkg/internal/tests/cross-tests/refresh.go (the only difference is *testing.T vs the T interface). When the fix PR lands and this helper is needed in more than two places, extracting it to a shared internal package (e.g., pkg/internal/tests/pulcheck) would prevent them from drifting.
1 participant
Summary
pulumi/pulumi-gcp#3666MaxItems=1 update failureMaxItems=1fieldNotes
tfbridgepanics when converting that state back into Pulumi outputsTesting
go test ./pkg/tests -run TestUpdateMaxItemsOneComputedNullStateParity -count=1 -vUnexpected multiple elements in array with MaxItems=1