From 9810dcd4b90e84c96adcfcb7c222e51a136d20d4 Mon Sep 17 00:00:00 2001 From: Liz Kammer Date: Tue, 28 Jul 2020 09:30:46 -0700 Subject: [PATCH 1/2] Update bpdocs to filter nested properties by tags (#312) * Update bpdocs to filter nested properties by tag Prior to this change nested properties tagged with `blueprint:"mutated"` are erroneously included in documentation, this corrects that behavior and adds testing to verify. Tested: go test bpdocs tests Change-Id: I822c9a98276634d2f584d8709e83003824cdffd5 --- bootstrap/bpdoc/bpdoc_test.go | 35 ++++++++++++++++++ bootstrap/bpdoc/properties.go | 1 + bootstrap/bpdoc/properties_test.go | 25 +++++++------ bootstrap/bpdoc/reader_test.go | 58 +++++++++++++++++++++++++----- 4 files changed, 100 insertions(+), 19 deletions(-) diff --git a/bootstrap/bpdoc/bpdoc_test.go b/bootstrap/bpdoc/bpdoc_test.go index 687d97b..3856933 100644 --- a/bootstrap/bpdoc/bpdoc_test.go +++ b/bootstrap/bpdoc/bpdoc_test.go @@ -1,6 +1,7 @@ package bpdoc import ( + "fmt" "reflect" "testing" ) @@ -44,3 +45,37 @@ func TestNestedPropertyStructs(t *testing.T) { } } } + +func TestAllPackages(t *testing.T) { + packages, err := AllPackages(pkgFiles, moduleTypeNameFactories, moduleTypeNamePropertyStructs) + if err != nil { + t.Errorf("expected nil error for AllPackages(%v, %v, %v), got %s", pkgFiles, moduleTypeNameFactories, moduleTypeNamePropertyStructs, err) + } + + if numPackages := len(packages); numPackages != 1 { + t.Errorf("Expected %d package, got %d packages %v instead", len(pkgFiles), numPackages, packages) + } + + pkg := packages[0] + + for _, m := range pkg.ModuleTypes { + for _, p := range m.PropertyStructs { + for _, err := range noMutatedProperties(p.Properties) { + t.Errorf("%s", err) + } + } + } +} + +func noMutatedProperties(properties []Property) []error { + errs := []error{} + for _, p := range properties { + if hasTag(p.Tag, "blueprint", "mutated") { + err := fmt.Errorf("Property %s has `blueprint:\"mutated\" tag but should have been excluded.", p) + errs = append(errs, err) + } + + errs = append(errs, noMutatedProperties(p.Properties)...) + } + return errs +} diff --git a/bootstrap/bpdoc/properties.go b/bootstrap/bpdoc/properties.go index 9256d8e..dcbb80e 100644 --- a/bootstrap/bpdoc/properties.go +++ b/bootstrap/bpdoc/properties.go @@ -251,6 +251,7 @@ func filterPropsByTag(props *[]Property, key, value string, exclude bool) { filtered := (*props)[:0] for _, x := range *props { if hasTag(x.Tag, key, value) == !exclude { + filterPropsByTag(&x.Properties, key, value, exclude) filtered = append(filtered, x) } } diff --git a/bootstrap/bpdoc/properties_test.go b/bootstrap/bpdoc/properties_test.go index 4045cb1..085bcdf 100644 --- a/bootstrap/bpdoc/properties_test.go +++ b/bootstrap/bpdoc/properties_test.go @@ -28,11 +28,8 @@ func TestExcludeByTag(t *testing.T) { ps.ExcludeByTag("tag1", "a") - expected := []string{"c"} - actual := []string{} - for _, p := range ps.Properties { - actual = append(actual, p.Name) - } + expected := []string{"c", "d", "g"} + actual := actualProperties(t, ps.Properties) if !reflect.DeepEqual(expected, actual) { t.Errorf("unexpected ExcludeByTag result, expected: %q, actual: %q", expected, actual) } @@ -47,12 +44,20 @@ func TestIncludeByTag(t *testing.T) { ps.IncludeByTag("tag1", "c") - expected := []string{"b", "c"} - actual := []string{} - for _, p := range ps.Properties { - actual = append(actual, p.Name) - } + expected := []string{"b", "c", "d", "f", "g"} + actual := actualProperties(t, ps.Properties) if !reflect.DeepEqual(expected, actual) { t.Errorf("unexpected IncludeByTag result, expected: %q, actual: %q", expected, actual) } } + +func actualProperties(t *testing.T, props []Property) []string { + t.Helper() + + actual := []string{} + for _, p := range props { + actual = append(actual, p.Name) + actual = append(actual, actualProperties(t, p.Properties)...) + } + return actual +} diff --git a/bootstrap/bpdoc/reader_test.go b/bootstrap/bpdoc/reader_test.go index 0d608b3..8493e14 100644 --- a/bootstrap/bpdoc/reader_test.go +++ b/bootstrap/bpdoc/reader_test.go @@ -16,6 +16,7 @@ package bpdoc import ( + "html/template" "reflect" "runtime" "testing" @@ -23,11 +24,29 @@ import ( "github.com/google/blueprint" ) +type factoryFn func() (blueprint.Module, []interface{}) + // foo docs. func fooFactory() (blueprint.Module, []interface{}) { return nil, []interface{}{&props{}} } +// bar docs. +func barFactory() (blueprint.Module, []interface{}) { + return nil, []interface{}{&complexProps{}} +} + +// for bpdoc_test.go +type complexProps struct { + A string + B_mutated string `blueprint:"mutated"` + + Nested struct { + C string + D_mutated string `blueprint:"mutated"` + } +} + // props docs. type props struct { // A docs. @@ -39,10 +58,18 @@ type tagTestProps struct { A string `tag1:"a,b" tag2:"c"` B string `tag1:"a,c"` C string `tag1:"b,c"` + + D struct { + E string `tag1:"a,b" tag2:"c"` + F string `tag1:"a,c"` + G string `tag1:"b,c"` + } `tag1:"b,c"` } var pkgPath string var pkgFiles map[string][]string +var moduleTypeNameFactories map[string]reflect.Value +var moduleTypeNamePropertyStructs map[string][]interface{} func init() { pc, filename, _, _ := runtime.Caller(0) @@ -57,21 +84,34 @@ func init() { pkgFiles = map[string][]string{ pkgPath: {filename}, } + + factories := map[string]factoryFn{"foo": fooFactory, "bar": barFactory} + + moduleTypeNameFactories = make(map[string]reflect.Value, len(factories)) + moduleTypeNamePropertyStructs = make(map[string][]interface{}, len(factories)) + for name, factory := range factories { + moduleTypeNameFactories[name] = reflect.ValueOf(factory) + _, structs := factory() + moduleTypeNamePropertyStructs[name] = structs + } } func TestModuleTypeDocs(t *testing.T) { r := NewReader(pkgFiles) - mt, err := r.ModuleType("foo_module", reflect.ValueOf(fooFactory)) - if err != nil { - t.Fatal(err) - } + for m := range moduleTypeNameFactories { + mt, err := r.ModuleType(m+"_module", moduleTypeNameFactories[m]) + if err != nil { + t.Fatal(err) + } - if mt.Text != "foo docs.\n\n" { - t.Errorf("unexpected docs %q", mt.Text) - } + expectedText := template.HTML(m + " docs.\n\n") + if mt.Text != expectedText { + t.Errorf("unexpected docs %q", mt.Text) + } - if mt.PkgPath != pkgPath { - t.Errorf("expected pkgpath %q, got %q", pkgPath, mt.PkgPath) + if mt.PkgPath != pkgPath { + t.Errorf("expected pkgpath %q, got %q", pkgPath, mt.PkgPath) + } } } From 5de3dc04e0eba32ce90f90ac0cb77c978faa9476 Mon Sep 17 00:00:00 2001 From: Daniel Cardenas Date: Thu, 30 Jul 2020 22:02:42 +0000 Subject: [PATCH 2/2] Android inclusive language fixit. Make comment more succinct, clear and remove use of term "sanity". Bug: 162536543 Change-Id: I4b40b1637e142d2f2fd513f62357213a9832bbaa --- module_ctx.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/module_ctx.go b/module_ctx.go index 9de2676..18dcdb0 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -1172,9 +1172,8 @@ func runAndRemoveLoadHooks(ctx *Context, config interface{}, module *moduleInfo, // Check the syntax of a generated blueprint file. // -// This is intended to perform a quick sanity check for generated blueprint -// code to ensure that it is syntactically correct, where syntactically correct -// means: +// This is intended to perform a quick syntactic check for generated blueprint +// code, where syntactically correct means: // * No variable definitions. // * Valid module types. // * Valid property names.