From 3d4dba4998059a465eabbe2e396599d54871cf56 Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Tue, 21 Sep 2021 00:01:30 +0200 Subject: [PATCH] schema,tests,gen/go: increase the coverage produced by the schema test suites markedly. Fixes a few old todos in the test system for typed nodes. The additional exercise picks up some bugs in the codegen for unions. (This was also reported by Adin, and I think possibly by Ian earlier, though I only got around to reproducing it fully now.) Those bugs -- off-by-ones; uuf -- are now fixed. Some todos were encountered in bindnode by the new coverage too. Only one thing. But it's a bit more than I knew how to fix in one sitting, so I've made that test skipped for now, hopefully to be fixed soon. Some error messages that confused me along the way are tweaked. The datamodel.DeepEquals method is slightly less fragile on nils. --- datamodel/equal.go | 3 ++ node/bindnode/node.go | 8 +++-- node/bindnode/schema_test.go | 3 ++ node/tests/testcase.go | 57 ++++++++++++++++++++++++++---- schema/gen/go/genUnion.go | 2 +- schema/gen/go/genUnionReprKeyed.go | 2 +- 6 files changed, 63 insertions(+), 12 deletions(-) diff --git a/datamodel/equal.go b/datamodel/equal.go index 2ad05f29..2a227b59 100644 --- a/datamodel/equal.go +++ b/datamodel/equal.go @@ -25,6 +25,9 @@ package datamodel // so an error should only happen if a Node implementation breaks that contract. // It is generally not recommended to call DeepEqual on ADL nodes. func DeepEqual(x, y Node) bool { + if x == nil || y == nil { + return false + } xk, yk := x.Kind(), y.Kind() if xk != yk { return false diff --git a/node/bindnode/node.go b/node/bindnode/node.go index de9ef0a7..e1d91b3c 100644 --- a/node/bindnode/node.go +++ b/node/bindnode/node.go @@ -754,11 +754,13 @@ func (w *_structAssembler) AssembleValue() datamodel.NodeAssembler { name := w.curKey.val.String() field := w.schemaType.Field(name) if field == nil { - panic(fmt.Sprintf("TODO: missing field %s in %s", name, w.schemaType.Name())) - // return nil, datamodel.ErrInvalidKey{ + // TODO: should've been raised when the key was submitted (we have room to return errors there, but can only panic at this point in the game). + // TODO: should make well-typed errors for this. + panic(fmt.Sprintf("TODO: invalid key: %q is not a field in type %s", name, w.schemaType.Name())) + // panic(schema.ErrInvalidKey{ // TypeName: w.schemaType.Name(), // Key: basicnode.NewString(name), - // } + // }) } ftyp, ok := w.val.Type().FieldByName(fieldNameFromSchema(name)) if !ok { diff --git a/node/bindnode/schema_test.go b/node/bindnode/schema_test.go index c67ae254..693f81a1 100644 --- a/node/bindnode/schema_test.go +++ b/node/bindnode/schema_test.go @@ -17,6 +17,9 @@ func forSchemaTest(name string) []tests.EngineSubtest { if name == "Links" { return nil // TODO(mvdan): add support for links } + if name == "UnionKeyedComplexChildren" { + return nil // Specifically, 'InhabitantB/repr-create_with_AK+AV' borks, because it needs representation-level AssignNode to support more. + } return []tests.EngineSubtest{{ Engine: &bindEngine{}, }} diff --git a/node/tests/testcase.go b/node/tests/testcase.go index 7d7acc2c..cae7af46 100644 --- a/node/tests/testcase.go +++ b/node/tests/testcase.go @@ -147,13 +147,6 @@ func (tcase testcase) Test(t *testing.T, np, npr datamodel.NodePrototype) { }) } - // Copy the node. This exercises the AssembleKey+AssembleValue path for maps, as opposed to the AssembleEntry path (which was exercised by the creation via unmarshal). - // This isn't especially informative for anything other than maps, but we do it universally anyway (it's not a significant time cost). - // TODO - - // Copy the node, now at repr level. Again, this is for exercising AssembleKey+AssembleValue paths. - // TODO - // Serialize the type-level node, and check that we get the original json again. // This exercises iterators on the type-level node. // OR, if typeItr is present, do that instead (this is necessary when handling maps with complex keys or handling structs with absent values, since both of those are unserializable). @@ -186,9 +179,59 @@ func (tcase testcase) Test(t *testing.T, np, npr datamodel.NodePrototype) { testMarshal(t, n.(schema.TypedNode).Representation(), tcase.reprJson) }) } + + // Copy the node. If it's a map-like. + // This exercises the AssembleKey+AssembleValue path for maps (or things that act as maps, such as structs and unions), + // as opposed to the AssembleEntry path (which is what was exercised by the creation via unmarshal). + // Assumes that the iterators are working correctly. + if n.Kind() == datamodel.Kind_Map { + t.Run("type-create with AK+AV", func(t *testing.T) { + n3, err := shallowCopyMap(np, n) + Wish(t, err, ShouldEqual, nil) + Wish(t, datamodel.DeepEqual(n, n3), ShouldEqual, true) + }) + } + + // Copy the node, now at repr level. Again, this is for exercising AssembleKey+AssembleValue paths. + // Assumes that the iterators are working correctly. + if n.(schema.TypedNode).Representation().Kind() == datamodel.Kind_Map { + t.Run("repr-create with AK+AV", func(t *testing.T) { + n3, err := shallowCopyMap(npr, n.(schema.TypedNode).Representation()) + Wish(t, err, ShouldEqual, nil) + Wish(t, datamodel.DeepEqual(n, n3), ShouldEqual, true) + }) + } + }) } +func shallowCopyMap(np datamodel.NodePrototype, n datamodel.Node) (datamodel.Node, error) { + nb := np.NewBuilder() + ma, err := nb.BeginMap(n.Length()) + if err != nil { + return nil, err + } + for itr := n.MapIterator(); !itr.Done(); { + k, v, err := itr.Next() + if err != nil { + return nil, err + } + if v.IsAbsent() { + continue + } + if err := ma.AssembleKey().AssignNode(k); err != nil { + return nil, err + } + if err := ma.AssembleValue().AssignNode(v); err != nil { + return nil, err + } + } + if err := ma.Finish(); err != nil { + return nil, err + } + return nb.Build(), nil +} + func testUnmarshal(t *testing.T, np datamodel.NodePrototype, data string, expectFail error) datamodel.Node { t.Helper() nb := np.NewBuilder() diff --git a/schema/gen/go/genUnion.go b/schema/gen/go/genUnion.go index 1eb342da..a185bbfd 100644 --- a/schema/gen/go/genUnion.go +++ b/schema/gen/go/genUnion.go @@ -540,7 +540,7 @@ func (g unionBuilderGenerator) emitMapAssemblerMethods(w io.Writer) { ma.state = maState_midValue switch ma.ca { {{- range $i, $member := .Type.Members }} - case {{ $i }}: + case {{ add $i 1 }}: {{- if (eq (dot.AdjCfg.UnionMemlayout dot.Type) "embedAll") }} ma.ca{{ add $i 1 }}.w = &ma.w.x{{ add $i 1 }} ma.ca{{ add $i 1 }}.m = &ma.cm diff --git a/schema/gen/go/genUnionReprKeyed.go b/schema/gen/go/genUnionReprKeyed.go index 44b6213a..c2af0253 100644 --- a/schema/gen/go/genUnionReprKeyed.go +++ b/schema/gen/go/genUnionReprKeyed.go @@ -404,7 +404,7 @@ func (g unionReprKeyedReprBuilderGenerator) emitMapAssemblerMethods(w io.Writer) ma.state = maState_midValue switch ma.ca { {{- range $i, $member := .Type.Members }} - case {{ $i }}: + case {{ add $i 1 }}: {{- if (eq (dot.AdjCfg.UnionMemlayout dot.Type) "embedAll") }} ma.ca{{ add $i 1 }}.w = &ma.w.x{{ add $i 1 }} ma.ca{{ add $i 1 }}.m = &ma.cm