Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c864b242 authored by Ronald Braunstein's avatar Ronald Braunstein
Browse files

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
parent 7694cdd6

android/all_teams.go

+17 −5
Original line number Diff line number Diff line
@@ -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,

android/all_teams_test.go

+54 −12
Original line number Diff line number Diff line
@@ -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,

		})

	}