From c864b242dabc91a9757616e10f7338e074354526 Mon Sep 17 00:00:00 2001 From: Ronald Braunstein Date: Tue, 16 Apr 2024 07:33:52 -0700 Subject: [PATCH] Prefer variants test-only:true attribute when grouping. When looking at more details of modules that are marked test-only, I saw that `java_test_host` modules were not in the list. The test I wrote for it passes, but in a real run, there are two variants (one windows, one linux) which causes it to fail. The `all_teams` code visis all variants, even not enabled ones. The windows variant, for which GenerateAndroidBuildActions was not being called, did not have a provider and its empty data was overriding the variant for which we had data. I changed the code to prefer variants where it is true. Generally for "test-only", the value is logically true independent of variant, so if one variant sets it true, it should be considered true for all variants. I think this is a slightly better check than preferring a variant with a provider or that is enabled. Prev CL % gqui from "flatten(~/aosp-main-with-phones/out/soong/ownership/all_teams.pb, teams)" proto team.proto:AllTeams 'select teams.kind, count(*) where teams.test_only = true and teams.kind not like "%cc_%" group by teams.kind' +--------------------------+----------+ | teams.kind | count(*) | +--------------------------+----------+ | android_test | 1382 | | android_test_helper_app | 1680 | | java_fuzz | 5 | | java_test | 774 | | java_test_helper_library | 29 | +--------------------------+----------+ After gqui from "flatten(~/aosp-main-with-phones/out/soong/ownership/all_teams.pb, teams)" proto ~/aosp-main-with-phones/build/soong/android/team_proto/team.proto:AllTeams ' select teams.kind, count(*) where teams.test_only = true and teams.kind not like "%cc_%" group by teams.kind' +--------------------------+----------+ | teams.kind | count(*) | +--------------------------+----------+ | android_test | 1382 | | android_test_helper_app | 1680 | | csuite_test | 16 | | java_fuzz | 10 | | java_test | 774 | | java_test_helper_library | 35 | | java_test_host | 495 | +--------------------------+----------+ Test: go test ./android Test: m all_teams Test: m blueprint_tests Change-Id: Idc5ed1c0375dc7390a0d58fcb4bf0d7fe1c7ab4f --- android/all_teams.go | 22 ++++++++++--- android/all_teams_test.go | 66 ++++++++++++++++++++++++++++++++------- 2 files changed, 71 insertions(+), 17 deletions(-) diff --git a/android/all_teams.go b/android/all_teams.go index 0c433a6cb..d4bf7d0c4 100644 --- a/android/all_teams.go +++ b/android/all_teams.go @@ -79,11 +79,6 @@ func (t *allTeamsSingleton) GenerateBuildActions(ctx SingletonContext) { ctx.VisitAllModules(func(module Module) { bpFile := ctx.BlueprintFile(module) - testModInfo := TestModuleInformation{} - if tmi, ok := SingletonModuleProvider(ctx, module, TestOnlyProviderKey); ok { - testModInfo = tmi - } - // Package Modules and Team Modules are stored in a map so we can look them up by name for // modules without a team. if pack, ok := module.(*packageModule); ok { @@ -97,6 +92,23 @@ func (t *allTeamsSingleton) GenerateBuildActions(ctx SingletonContext) { return } + testModInfo := TestModuleInformation{} + if tmi, ok := SingletonModuleProvider(ctx, module, TestOnlyProviderKey); ok { + testModInfo = tmi + } + + // Some modules, like java_test_host don't set the provider when the module isn't enabled: + // test_only, top_level + // AVFHostTestCases{os:linux_glibc,arch:common} {true true} + // AVFHostTestCases{os:windows,arch:common} {false false} + // Generally variant information of true override false or unset. + if testModInfo.TestOnly == false { + if prevValue, exists := t.teams_for_mods[module.Name()]; exists { + if prevValue.testOnly == true { + return + } + } + } entry := moduleTeamAndTestInfo{ bpFile: bpFile, testOnly: testModInfo.TestOnly, diff --git a/android/all_teams_test.go b/android/all_teams_test.go index 9c2b38e42..96ed92fc5 100644 --- a/android/all_teams_test.go +++ b/android/all_teams_test.go @@ -25,6 +25,8 @@ func TestAllTeams(t *testing.T) { t.Parallel() ctx := GroupFixturePreparers( prepareForTestWithTeamAndFakes, + // This adds two variants, one armv7-a-neon, one armv8-a + PrepareForTestWithArchMutator, FixtureRegisterWithContext(func(ctx RegistrationContext) { ctx.RegisterParallelSingletonType("all_teams", AllTeamsFactory) }), @@ -52,10 +54,35 @@ func TestAllTeams(t *testing.T) { name: "noteam", test_only: true, } + // write the test-only provider value once fake { - name: "test-and-team-and-top", + name: "test-and-team-and-top1", test_only: true, team: "team2", + arch: {arm: { skip: false}, + arm64: { skip: true}}, + } + // write the test-only provider once, but on the other arch + fake { + name: "test-and-team-and-top2", + test_only: true, + team: "team2", + arch: {arm: { skip: true}, + arm64: { skip: false}}, + } + // write the test-only provider value twice + fake { + name: "test-and-team-and-top3", + test_only: true, + team: "team2", + } + // Don't write the test-only provider value + fake { + name: "test-and-team-and-top4", + test_only: true, + team: "team2", + arch: {arm: { skip: true}, + arm64: { skip: true}}, } `) @@ -63,12 +90,16 @@ func TestAllTeams(t *testing.T) { teams = getTeamProtoOutput(t, ctx) // map of module name -> trendy team name. - actualTeams := make(map[string]*string) + actualTeams := make(map[string]string) actualTests := []string{} actualTopLevelTests := []string{} for _, teamProto := range teams.Teams { - actualTeams[teamProto.GetTargetName()] = teamProto.TrendyTeamId + if teamProto.TrendyTeamId != nil { + actualTeams[teamProto.GetTargetName()] = *teamProto.TrendyTeamId + } else { + actualTeams[teamProto.GetTargetName()] = "" + } if teamProto.GetTestOnly() { actualTests = append(actualTests, teamProto.GetTargetName()) } @@ -76,16 +107,23 @@ func TestAllTeams(t *testing.T) { actualTopLevelTests = append(actualTopLevelTests, teamProto.GetTargetName()) } } - expectedTeams := map[string]*string{ - "main_test": proto.String("cool_team"), - "tool": proto.String("22222"), - "test-and-team-and-top": proto.String("22222"), - "noteam": nil, + expectedTeams := map[string]string{ + "main_test": "cool_team", + "tool": "22222", + "test-and-team-and-top1": "22222", + "test-and-team-and-top2": "22222", + "test-and-team-and-top3": "22222", + "test-and-team-and-top4": "22222", + "noteam": "", } expectedTests := []string{ "noteam", - "test-and-team-and-top", + "test-and-team-and-top1", + "test-and-team-and-top2", + "test-and-team-and-top3", + // There should be no test-and-team-top4 as we skip writing all variants + // test-only for all variants } AssertDeepEquals(t, "compare maps", expectedTeams, actualTeams) AssertDeepEquals(t, "test matchup", expectedTests, actualTests) @@ -230,12 +268,16 @@ type fakeForTests struct { ModuleBase sourceProperties SourceProperties + props struct { + // If true, don't write test-only value in provider + Skip bool `android:"arch_variant"` + } } func fakeFactory() Module { module := &fakeForTests{} - module.AddProperties(&module.sourceProperties) - InitAndroidModule(module) + module.AddProperties(&module.sourceProperties, &module.props) + InitAndroidArchModule(module, HostAndDeviceSupported, MultilibBoth) return module } @@ -250,7 +292,7 @@ var prepareForTestWithTeamAndFakes = GroupFixturePreparers( func (f *fakeForTests) GenerateAndroidBuildActions(ctx ModuleContext) { if Bool(f.sourceProperties.Test_only) { SetProvider(ctx, TestOnlyProviderKey, TestModuleInformation{ - TestOnly: Bool(f.sourceProperties.Test_only), + TestOnly: Bool(f.sourceProperties.Test_only) && !f.props.Skip, TopLevelTarget: false, }) }