Skip to content

Commit 0cfdbc8

Browse files
committed
test(osgen): pin perOpErrorTypeName <-> errwrap.OperationWrappers coupling
perOpErrorTypeName's hardcoded switch and errwrap.OperationWrappers' wrapper-count map are coupled by an unstated invariant: a group has a per-op aggregator type iff its catalog entry declares 2+ wrappers. Today both sides match, but nothing checks them, so a future catalog edit can desync the two without any signal -- the dispatch keeps referencing a per-op type that's no longer reachable, or worse, emits an empty type name when a 2+-wrapper group lacks a switch arm. Add a coupling test that asserts both directions: - every group naming a per-op aggregator type has 2+ wrappers in OperationWrappers - every catalog entry with 2+ wrappers has a non-empty per-op aggregator type Iterates the catalog directly rather than a duplicate list of switch arms, so a new switch arm or catalog entry is exercised automatically. Failure messages are actionable: they name the offending group, the current state, and the remediation (add wrappers, remove switch arm, or add a hand-written aggregator). Ref: opensearch-project#844 (review round 3, F7) Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
1 parent f95f305 commit 0cfdbc8

1 file changed

Lines changed: 46 additions & 0 deletions

File tree

cmd/osgen/emit/frag_dispatch_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,52 @@ func TestDispatchFragment_Imports(t *testing.T) {
306306
}
307307
}
308308

309+
// TestPerOpErrorTypeName_CatalogConsistency pins the implicit coupling
310+
// between [emit.PerOpErrorTypeName]'s hardcoded switch and
311+
// [errwrap.OperationWrappers]: every group naming a per-op aggregator
312+
// type must declare 2+ wrappers in the catalog (otherwise the per-op
313+
// type is referenced from generated code but the underlying detection
314+
// paths no longer fire), and every group declaring 2+ wrappers in the
315+
// catalog must have a per-op aggregator type (otherwise the dispatch
316+
// emits an empty per-op type name).
317+
//
318+
// Without this test, a future catalog edit could desync the two
319+
// without any signal until generated code stops compiling.
320+
func TestPerOpErrorTypeName_CatalogConsistency(t *testing.T) {
321+
t.Parallel()
322+
323+
// Forward direction: every group that names a per-op aggregator type
324+
// must have 2+ wrappers in the catalog. Iterating the catalog itself
325+
// (rather than a duplicate list of switch arms) means a new switch
326+
// arm is exercised the moment any group ever appears in either side.
327+
for group := range errwrap.OperationWrappers {
328+
typeName := emit.PerOpErrorTypeName(group)
329+
if typeName == "" {
330+
continue
331+
}
332+
t.Run("type_for_"+group, func(t *testing.T) {
333+
t.Parallel()
334+
require.GreaterOrEqual(t, len(errwrap.OperationWrappers[group]), 2,
335+
"group %q has per-op error type %q but only %d wrapper(s) in OperationWrappers; either add wrappers or remove the switch arm",
336+
group, typeName, len(errwrap.OperationWrappers[group]))
337+
})
338+
}
339+
340+
// Reverse direction: every catalog entry with 2+ wrappers must name
341+
// a per-op aggregator type.
342+
for group, wrappers := range errwrap.OperationWrappers {
343+
if len(wrappers) < 2 {
344+
continue
345+
}
346+
t.Run("catalog_entry_"+group, func(t *testing.T) {
347+
t.Parallel()
348+
require.NotEmpty(t, emit.PerOpErrorTypeName(group),
349+
"group %q declares %d wrappers %v in OperationWrappers but PerOpErrorTypeName returns empty; add a switch arm and a hand-written %q-style aggregator type",
350+
group, len(wrappers), wrappers, group)
351+
})
352+
}
353+
}
354+
309355
// ---------------------------------------------------------------------------
310356
// PartialFailureFragment: per-Resp helper methods + aggregator
311357
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)