Skip to content

Commit 7d9b3f5

Browse files
committed
docs(test): explain why txdb can't seed shared state across subtests
Expand the comment on TestShell_BeforeNode and TestShell_RunNode_WithBeforeNode to document *why* heavyweight.FullTestDBV2 is the right tool here, not txdb. The subtests need to share a seeded encrypted key ring so the incorrect_password case has something to fail decryption against (keystore.Unlock on an empty keystore silently creates a new ring with whatever password is supplied). Naively you would reach for txdb, but chainlink-common/pkg/sqlutil/pg/pg.go:69 passes uuid.New().String() as the DSN on every pg.NewConnection call — deliberately, so each ORM gets its own transaction — which means the seed connection and BeforeNode's LockedDB land on different keys in the txdb driver's DSN-keyed conns map and get independent transactions. Seed is invisible to BeforeNode. Same reason Erik's PR #21504 newAppWithOpts + txdb override couldn't make incorrect_password pass. A real per-test physical DB via FullTestDBV2 gives cross-connection visibility with its own t.Cleanup and without polluting the shared chainlink_test DB. No functional change — just documents the trap so the next person doesn't repeat the investigation. Refs: CORE-2388
1 parent 221be77 commit 7d9b3f5

1 file changed

Lines changed: 29 additions & 13 deletions

File tree

core/cmd/shell_local_test.go

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -509,15 +509,31 @@ func TestShell_RemoveBlocks(t *testing.T) {
509509
func TestShell_BeforeNode(t *testing.T) {
510510
testutils.SkipShortDB(t)
511511

512-
// Use a real isolated DB (dropped on t.Cleanup via heavyweight.FullTestDBV2)
513-
// so both subtests share persisted keystore state. This is required because:
514-
// 1. keystore.Unlock on an *empty* keystore silently creates a fresh key
515-
// ring encrypted with whatever password you pass — so any password
516-
// "works" against an empty DB, including the wrong one.
517-
// 2. The incorrect_password subtest can only meaningfully fail if there
518-
// is an existing encrypted key ring to fail decryption against.
519-
// txdb isolation would roll back the seed between subtests, so we use a
520-
// persistent per-test DB instead.
512+
// Use heavyweight.FullTestDBV2 (real isolated per-test Postgres DB,
513+
// dropped on t.Cleanup) because both subtests must share a persisted
514+
// encrypted key ring:
515+
//
516+
// 1. keystore.Unlock on an *empty* keystore silently creates a fresh
517+
// key ring encrypted with whatever password you pass — so any
518+
// password "works" against an empty DB, including the wrong one.
519+
// The incorrect_password subtest can only fail meaningfully if
520+
// there is an existing ring to fail decryption against.
521+
//
522+
// 2. txdb isolation can't be used here: chainlink-common's
523+
// pg.NewConnection deliberately passes uuid.New().String() as the
524+
// DSN to the txdb driver on every call (see
525+
// chainlink-common/pkg/sqlutil/pg/pg.go:69 — "Each ORM should be
526+
// encapsulated in its own transaction"). That means the seed
527+
// connection and BeforeNode's LockedDB land on different keys in
528+
// the txdb driver's DSN-keyed conns map and get independent
529+
// transactions, so the seed would be invisible to BeforeNode.
530+
// This convention is also why Erik's PR #21504, which tried to
531+
// force txdb via a newAppWithOpts override, couldn't make the
532+
// incorrect_password case work.
533+
//
534+
// A real per-test physical DB gives us the cross-connection visibility
535+
// the test needs, with its own cleanup and without polluting the
536+
// shared chainlink_test DB used by every other test.
521537
cfg, sqlxDB := heavyweight.FullTestDBV2(t, func(c *chainlink.Config, s *chainlink.Secrets) {
522538
s.Password.Keystore = models.NewSecret("dummy-to-pass-validation")
523539
c.EVM[0].Nodes[0].Name = ptr("fake")
@@ -595,10 +611,10 @@ func TestShell_BeforeNode(t *testing.T) {
595611
func TestShell_RunNode_WithBeforeNode(t *testing.T) {
596612
testutils.SkipShortDB(t)
597613

598-
// See TestShell_BeforeNode for why we need a real shared DB with a seeded
599-
// key ring: keystore.Unlock on an empty keystore silently creates keys
600-
// with whatever password is passed, so an empty DB can't meaningfully
601-
// test the incorrect_password case.
614+
// See TestShell_BeforeNode for the full rationale (TL;DR: txdb can't
615+
// be used to seed shared state because pg.NewConnection generates a
616+
// fresh UUID DSN per call, isolating every pool into its own
617+
// transaction — so we need a real per-test physical DB instead).
602618
cfg, sqlxDB := heavyweight.FullTestDBV2(t, func(c *chainlink.Config, s *chainlink.Secrets) {
603619
s.Password.Keystore = models.NewSecret("dummy-to-pass-validation")
604620
c.EVM[0].Nodes[0].Name = ptr("fake")

0 commit comments

Comments
 (0)