From 545c59273de8e741ca8820656d972fbeb03f1ef9 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Thu, 27 Jan 2022 16:51:51 +0000 Subject: [PATCH] Refactor build_release and test code Minor restructuring of the build_release pruning code to make it easier to add logic for handling fields of maps of structs. That includes: 1. Moving some code that is specific to clearing a selected field inside the associated if block. 2. Replacing an if with a switch. 3. Improving the error handling by separating the reporting of the container that broke from information about which field could not be set. That allows follow up code to provide information about the map key instead. The tests were restructed by: 1. Switching from using AssertDeepEquals to compare the structs to comparing the output of marshalling the structs to JSON. That was for a couple of reasons. Firstly, because JSON will marshal (and so allow comparison of) the contents of pointers to structs whereas AssertDeepEquals will just compare the pointers themselves. Secondly, because JSON can pretty print the output and make it easier to read. 2. Using a func to create a new instance of the input structure for each test. That is to allow the test to modify the input structure, e.g. by clearing a field in a struct that is pointed to by a map. The test previously relied on the input structure being immutable and passed by value but a follow up change will change that by adding a map field that contains pointers to structs. Bug: 204763318 Test: m nothing Change-Id: I84dc99621497b7263e30466895b823eb02cb2b56 --- sdk/build_release.go | 70 +++++++++++++++++++++++---------------- sdk/build_release_test.go | 58 +++++++++++++++++++++----------- 2 files changed, 80 insertions(+), 48 deletions(-) diff --git a/sdk/build_release.go b/sdk/build_release.go index a3f089973..212582ac6 100644 --- a/sdk/build_release.go +++ b/sdk/build_release.go @@ -230,51 +230,63 @@ func (p *propertyPruner) gatherFields(structType reflect.Type, containingStructA return container.Field(fieldIndex) } - zeroValue := reflect.Zero(field.Type) - fieldPruner := func(container reflect.Value) { - if containingStructAccessor != nil { - // This is an embedded structure so first access the field for the embedded - // structure. - container = containingStructAccessor(container) + fieldType := field.Type + if selector(name, field) { + zeroValue := reflect.Zero(fieldType) + fieldPruner := func(container reflect.Value) { + if containingStructAccessor != nil { + // This is an embedded structure so first access the field for the embedded + // structure. + container = containingStructAccessor(container) + } + + // Skip through interface and pointer values to find the structure. + container = getStructValue(container) + + defer func() { + if r := recover(); r != nil { + panic(fmt.Errorf("%s\n\tfor field (index %d, name %s)", r, fieldIndex, name)) + } + }() + + // Set the field. + container.Field(fieldIndex).Set(zeroValue) } - // Skip through interface and pointer values to find the structure. - container = getStructValue(container) - - defer func() { - if r := recover(); r != nil { - panic(fmt.Errorf("%s for fieldIndex %d of field %s of container %#v", r, fieldIndex, name, container.Interface())) - } - }() - - // Set the field. - container.Field(fieldIndex).Set(zeroValue) - } - - if selector(name, field) { property := prunerProperty{ name, fieldPruner, } p.properties = append(p.properties, property) - } else if field.Type.Kind() == reflect.Struct { - // Gather fields from the nested or embedded structure. - var subNamePrefix string - if field.Anonymous { - subNamePrefix = namePrefix - } else { - subNamePrefix = name + "." + } else { + switch fieldType.Kind() { + case reflect.Struct: + // Gather fields from the nested or embedded structure. + var subNamePrefix string + if field.Anonymous { + subNamePrefix = namePrefix + } else { + subNamePrefix = name + "." + } + p.gatherFields(fieldType, fieldGetter, subNamePrefix, selector) } - p.gatherFields(field.Type, fieldGetter, subNamePrefix, selector) } } } -// pruneProperties will prune (set to zero value) any properties in the supplied struct. +// pruneProperties will prune (set to zero value) any properties in the struct referenced by the +// supplied struct pointer. // // The struct must be of the same type as was originally passed to newPropertyPruner to create this // propertyPruner. func (p *propertyPruner) pruneProperties(propertiesStruct interface{}) { + + defer func() { + if r := recover(); r != nil { + panic(fmt.Errorf("%s\n\tof container %#v", r, propertiesStruct)) + } + }() + structValue := reflect.ValueOf(propertiesStruct) for _, property := range p.properties { property.prunerFunc(structValue) diff --git a/sdk/build_release_test.go b/sdk/build_release_test.go index dff276d0c..0ec10409f 100644 --- a/sdk/build_release_test.go +++ b/sdk/build_release_test.go @@ -15,6 +15,7 @@ package sdk import ( + "encoding/json" "fmt" "testing" @@ -132,54 +133,73 @@ func TestPropertyPrunerByBuildRelease(t *testing.T) { Nested nested } - input := testBuildReleasePruner{ - Default: "Default", - S_and_T_only: "S_and_T_only", - T_later: "T_later", - Nested: nested{ - F1_only: "F1_only", - }, + inputFactory := func() testBuildReleasePruner { + return testBuildReleasePruner{ + Default: "Default", + S_and_T_only: "S_and_T_only", + T_later: "T_later", + Nested: nested{ + F1_only: "F1_only", + }, + } + } + + marshal := func(t interface{}) string { + bytes, err := json.MarshalIndent(t, "", " ") + if err != nil { + panic(err) + } + return string(bytes) + } + + assertJsonEquals := func(t *testing.T, expected, actual interface{}) { + t.Helper() + expectedJson := marshal(expected) + actualJson := marshal(actual) + if actualJson != expectedJson { + t.Errorf("test struct: expected:\n%s\n got:\n%s", expectedJson, actualJson) + } } t.Run("target S", func(t *testing.T) { - testStruct := input + testStruct := inputFactory() pruner := newPropertyPrunerByBuildRelease(&testStruct, buildReleaseS) pruner.pruneProperties(&testStruct) - expected := input + expected := inputFactory() expected.T_later = "" expected.Nested.F1_only = "" - android.AssertDeepEquals(t, "test struct", expected, testStruct) + assertJsonEquals(t, expected, testStruct) }) t.Run("target T", func(t *testing.T) { - testStruct := input + testStruct := inputFactory() pruner := newPropertyPrunerByBuildRelease(&testStruct, buildReleaseT) pruner.pruneProperties(&testStruct) - expected := input + expected := inputFactory() expected.Nested.F1_only = "" - android.AssertDeepEquals(t, "test struct", expected, testStruct) + assertJsonEquals(t, expected, testStruct) }) t.Run("target F1", func(t *testing.T) { - testStruct := input + testStruct := inputFactory() pruner := newPropertyPrunerByBuildRelease(&testStruct, buildReleaseFuture1) pruner.pruneProperties(&testStruct) - expected := input + expected := inputFactory() expected.S_and_T_only = "" - android.AssertDeepEquals(t, "test struct", expected, testStruct) + assertJsonEquals(t, expected, testStruct) }) t.Run("target F2", func(t *testing.T) { - testStruct := input + testStruct := inputFactory() pruner := newPropertyPrunerByBuildRelease(&testStruct, buildReleaseFuture2) pruner.pruneProperties(&testStruct) - expected := input + expected := inputFactory() expected.S_and_T_only = "" expected.Nested.F1_only = "" - android.AssertDeepEquals(t, "test struct", expected, testStruct) + assertJsonEquals(t, expected, testStruct) }) }