Skip to content

Commit fb0001a

Browse files
committed
PR(FIX): Skip change-detector tests with any uncommitted transaction
getActionRange only tracked transactions opened by AddCollection/AddDoc/ UpdateDoc, so a test that opened a transaction from any other action (e.g. DeleteCollection) and never committed it slipped past the open-transaction skip and ran with a broken setup/assert split. Detect open transactions across every action via its TransactionID field instead.
1 parent 9b03232 commit fb0001a

1 file changed

Lines changed: 55 additions & 33 deletions

File tree

tests/integration/utils.go

Lines changed: 55 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -794,44 +794,29 @@ func getActionRange(t testing.TB, testCase TestCase) (int, int) {
794794
return startIndex, endIndex
795795
}
796796

797+
// A test that leaves a transaction open cannot be run by the change detector:
798+
// uncommitted writes are never persisted, so the setup and assert phases would
799+
// operate on different data. Any action can run inside a transaction - not just
800+
// the mutation actions that delimit the setup phase below - so this is checked
801+
// across every action, independently of where they are split.
802+
if hasOpenTransaction(testCase.Actions) {
803+
t.Skipf("skipping test with open transaction(s)")
804+
}
805+
797806
setupCompleteIndex := -1
798807
firstNonSetupIndex := -1
799808

800-
// Track the transaction IDs as they are used, in a set
801-
transactionIDset := make(map[int]struct{})
802-
803809
ActionLoop:
804810
for i := range testCase.Actions {
805-
switch concreteAction := testCase.Actions[i].(type) {
811+
switch testCase.Actions[i].(type) {
806812
case SetupComplete:
807813
setupCompleteIndex = i
808814
// We don't care about anything else if this has been explicitly provided
809815
break ActionLoop
810816

811-
case *action.AddCollection:
812-
if concreteAction.TransactionID.HasValue() {
813-
transactionIDset[concreteAction.TransactionID.Value()] = struct{}{}
814-
}
815-
continue
816-
817-
case *action.AddDoc:
818-
if concreteAction.TransactionID.HasValue() {
819-
transactionIDset[concreteAction.TransactionID.Value()] = struct{}{}
820-
}
821-
continue
822-
823-
case *action.UpdateDoc:
824-
if concreteAction.TransactionID.HasValue() {
825-
transactionIDset[concreteAction.TransactionID.Value()] = struct{}{}
826-
}
827-
continue
828-
829-
case Restart:
830-
continue
831-
832-
case *action.CommitTransaction:
833-
// If transaction is commited, remove it from the set we are tracking
834-
delete(transactionIDset, concreteAction.TransactionID)
817+
// Mutation actions form the setup phase; Restart and CommitTransaction are
818+
// permitted to interleave with them without ending it.
819+
case *action.AddCollection, *action.AddDoc, *action.UpdateDoc, Restart, *action.CommitTransaction:
835820
continue
836821

837822
default:
@@ -840,11 +825,6 @@ ActionLoop:
840825
}
841826
}
842827

843-
// If length is not 0, there was a transaction used that was not committed
844-
if len(transactionIDset) > 0 {
845-
t.Skipf("skipping test with open transaction(s)")
846-
}
847-
848828
if changeDetector.SetupOnly {
849829
if setupCompleteIndex > -1 {
850830
endIndex = setupCompleteIndex
@@ -870,6 +850,48 @@ ActionLoop:
870850
return startIndex, endIndex
871851
}
872852

853+
// hasOpenTransaction reports whether the actions open a transaction that is never
854+
// committed. Any action may run inside a transaction via an optional TransactionID
855+
// field; a CommitTransaction closes the one it names.
856+
func hasOpenTransaction(actions []any) bool {
857+
open := make(map[int]struct{})
858+
for _, a := range actions {
859+
if commit, ok := a.(*action.CommitTransaction); ok {
860+
delete(open, commit.TransactionID)
861+
continue
862+
}
863+
if id, ok := actionTransactionID(a); ok {
864+
open[id] = struct{}{}
865+
}
866+
}
867+
return len(open) > 0
868+
}
869+
870+
// actionTransactionID returns the transaction an action runs in, read from its
871+
// optional `TransactionID immutable.Option[int]` field if it has one. Reflection
872+
// keeps this correct for every such action; an exhaustive type switch would
873+
// silently miss any action added later - the gap this guards against.
874+
func actionTransactionID(a any) (int, bool) {
875+
v := reflect.ValueOf(a)
876+
for v.Kind() == reflect.Pointer {
877+
v = v.Elem()
878+
}
879+
if v.Kind() != reflect.Struct {
880+
return 0, false
881+
}
882+
883+
field := v.FieldByName("TransactionID")
884+
if !field.IsValid() {
885+
return 0, false
886+
}
887+
888+
txnID, ok := field.Interface().(immutable.Option[int])
889+
if !ok || !txnID.HasValue() {
890+
return 0, false
891+
}
892+
return txnID.Value(), true
893+
}
894+
873895
// setStartingNodes adds a set of initial Defra nodes for the test to execute against.
874896
//
875897
// If a node(s) has been explicitly configured via a `ConfigureNode` action then no new

0 commit comments

Comments
 (0)