From 1a6e7c032a0c4acda17e38c59bda932d5f7a1e2f Mon Sep 17 00:00:00 2001 From: Ronald Braunstein Date: Thu, 14 Mar 2024 21:14:39 +0000 Subject: [PATCH] Add test_module_config_host This pairs with `test_module_config` but also works on the base is a `java_test_host` module. e.g. test_module_config_host { name: "CtsOsHostTestCases_DERIVED_2566", base: "CtsOsHostTestCases", test_suites: ["general-tests"], include_filters: [ "android.os.cts.StaticSharedLibsHostTests" ], exclude_annotations: [ "androidx.test.filters.FlakyTest","org.junit.Ignore" ], } The new module is composed of the previous and shares much of the same code. With respect to build size, Without this change, if you build CtsAppSecurityHostTestCases, there will be several copies of the jar (and related apks) : *) 1 in framework out/host/linux-x86/framework/CtsAppSecurityHostTestCases.jar *) 1 in testcases for the test out/host/linux-x86/testcases/CtsAppSecurityHostTestCases/CtsAppSecurityHostTestCases.jar *) 1 per compatibility suite testcases out/host/linux-x86/mts-documentsui/android-mts-documentsui/testcases/CtsAppSecurityHostTestCases/CtsAppSecurityHostTestCases.jar out/host/linux-x86/mts-mediaprovider/android-mts-mediaprovider/testcases/CtsAppSecurityHostTestCases/CtsAppSecurityHostTestCases.jar out/host/linux-x86/mts/android-mts/testcases/CtsAppSecurityHostTestCases/CtsAppSecurityHostTestCases.jar out/host/linux-x86/cts/android-cts/testcases/CtsAppSecurityHostTestCases/CtsAppSecurityHostTestCases.jar out/host/linux-x86/mts-mainline-infra/android-mts-mainline-infra/testcases/CtsAppSecurityHostTestCases/CtsAppSecurityHostTestCases.jar A dervived test using CtsAppSecurityHostTestCases as base adds one more to its testcases dir: *) derived testcase. out/host/linux-x86/testcases/CtsAppSecurityHostTestCases_presubmit_ExternalStorageApp/CtsAppSecurityHostTestCases.jar Fixes: b/327280990 Test: m clean && m CtsOsHostTestCases_DERIVED_2566# as above Test: atest CtsOsHostTestCases_DERIVED_2566 --collect-tests-only Test: migrated the 71 TestMapping instances to Android.bp and build them. Ran tests on some of them. Ran some original `test_module_config` tests derived from `android_test` as well TODO: Add actions to validate the given filters are valid for the given test apks/jars. Change-Id: I115eedb6ff6ba8e72bb49e71867daf49d25ca0f1 --- java/app.go | 2 + java/java.go | 8 + tradefed/providers.go | 5 + tradefed_modules/test_module_config.go | 180 +++++++++++++++----- tradefed_modules/test_module_config_test.go | 165 +++++++++++++++++- 5 files changed, 317 insertions(+), 43 deletions(-) diff --git a/java/app.go b/java/app.go index 4f6f1f3e8..57d3490ce 100755 --- a/java/app.go +++ b/java/app.go @@ -1328,6 +1328,8 @@ func (a *AndroidTest) GenerateAndroidBuildActions(ctx android.ModuleContext) { OutputFile: a.OutputFile(), TestConfig: a.testConfig, HostRequiredModuleNames: a.HostRequiredModuleNames(), + TestSuites: a.testProperties.Test_suites, + IsHost: false, }) } diff --git a/java/java.go b/java/java.go index e606993f2..d5a85f749 100644 --- a/java/java.go +++ b/java/java.go @@ -1433,6 +1433,14 @@ func (j *TestHost) GenerateAndroidBuildActions(ctx android.ModuleContext) { j.Test.generateAndroidBuildActionsWithConfig(ctx, configs) android.SetProvider(ctx, testing.TestModuleProviderKey, testing.TestModuleProviderData{}) + android.SetProvider(ctx, tradefed.BaseTestProviderKey, tradefed.BaseTestProviderData{ + InstalledFiles: j.data, + OutputFile: j.outputFile, + TestConfig: j.testConfig, + RequiredModuleNames: j.RequiredModuleNames(), + TestSuites: j.testProperties.Test_suites, + IsHost: true, + }) } func (j *Test) GenerateAndroidBuildActions(ctx android.ModuleContext) { diff --git a/tradefed/providers.go b/tradefed/providers.go index f41e09eb6..66cb6253b 100644 --- a/tradefed/providers.go +++ b/tradefed/providers.go @@ -16,6 +16,11 @@ type BaseTestProviderData struct { TestConfig android.Path // Other modules we require to be installed to run tests. We expect base to build them. HostRequiredModuleNames []string + RequiredModuleNames []string + // List of test suites base uses. + TestSuites []string + // Used for bases that are Host + IsHost bool } var BaseTestProviderKey = blueprint.NewProvider[BaseTestProviderData]() diff --git a/tradefed_modules/test_module_config.go b/tradefed_modules/test_module_config.go index ba6ab9400..686753713 100644 --- a/tradefed_modules/test_module_config.go +++ b/tradefed_modules/test_module_config.go @@ -17,6 +17,7 @@ func init() { // Register the license_kind module type. func RegisterTestModuleConfigBuildComponents(ctx android.RegistrationContext) { ctx.RegisterModuleType("test_module_config", TestModuleConfigFactory) + ctx.RegisterModuleType("test_module_config_host", TestModuleConfigHostFactory) } type testModuleConfigModule struct { @@ -32,6 +33,12 @@ type testModuleConfigModule struct { provider tradefed.BaseTestProviderData } +// Host is mostly the same as non-host, just some diffs for AddDependency and +// AndroidMkEntries, but the properties are the same. +type testModuleConfigHostModule struct { + testModuleConfigModule +} + // Properties to list in Android.bp for this module. type tradefedProperties struct { // Module name of the base test that we will run. @@ -68,8 +75,9 @@ type dependencyTag struct { } var ( - testModuleConfigTag = dependencyTag{name: "TestModuleConfigBase"} - pctx = android.NewPackageContext("android/soong/tradefed_modules") + testModuleConfigTag = dependencyTag{name: "TestModuleConfigBase"} + testModuleConfigHostTag = dependencyTag{name: "TestModuleConfigHostBase"} + pctx = android.NewPackageContext("android/soong/tradefed_modules") ) func (m *testModuleConfigModule) InstallInTestcases() bool { @@ -77,6 +85,10 @@ func (m *testModuleConfigModule) InstallInTestcases() bool { } func (m *testModuleConfigModule) DepsMutator(ctx android.BottomUpMutatorContext) { + if m.Base == nil { + ctx.ModuleErrorf("'base' field must be set to a 'android_test' module.") + return + } ctx.AddDependency(ctx.Module(), testModuleConfigTag, *m.Base) } @@ -143,35 +155,41 @@ func (m *testModuleConfigModule) composeOptions() []tradefed.Option { // // If we change to symlinks, this all needs to change. func (m *testModuleConfigModule) GenerateAndroidBuildActions(ctx android.ModuleContext) { + m.validateBase(ctx, &testModuleConfigTag, "android_test", false) + m.generateManifestAndConfig(ctx) - ctx.VisitDirectDepsWithTag(testModuleConfigTag, func(dep android.Module) { - if provider, ok := android.OtherModuleProvider(ctx, dep, tradefed.BaseTestProviderKey); ok { - m.base = dep - m.provider = provider - } else { - ctx.ModuleErrorf("The base module '%s' does not provide test BaseTestProviderData. Only 'android_test' modules are supported.", dep.Name()) - return +} + +// Any test suites in base should not be repeated in the derived class, except "general-tests". +// We may restrict derived tests to only be "general-tests" as it doesn't make sense to add a slice +// of a test to compatibility suite. +// +// Returns ErrorMessage, false on problems +// Returns _, true if okay. +func (m *testModuleConfigModule) validateTestSuites(ctx android.ModuleContext) bool { + if len(m.tradefedProperties.Test_suites) == 0 { + ctx.ModuleErrorf("At least one test-suite must be set or this won't run. Use \"general-tests\"") + return false + } + + derivedSuites := make(map[string]bool) + // See if any suites in base is also in derived (other than general-tests) + for _, s := range m.tradefedProperties.Test_suites { + if s != "general-tests" { + derivedSuites[s] = true } - }) + } + if len(derivedSuites) == 0 { + return true + } + for _, baseSuite := range m.provider.TestSuites { + if derivedSuites[baseSuite] { + ctx.ModuleErrorf("TestSuite %s exists in the base, do not add it here", baseSuite) + return false + } + } - // 1) A manifest file listing the base. - installDir := android.PathForModuleInstall(ctx, ctx.ModuleName()) - out := android.PathForModuleOut(ctx, "test_module_config.manifest") - android.WriteFileRule(ctx, out, fmt.Sprintf("{%q: %q}", "base", *m.tradefedProperties.Base)) - ctx.InstallFile(installDir, out.Base(), out) - - // 2) Module.config / AndroidTest.xml - // Note, there is still a "test-tag" element with base's module name, but - // Tradefed team says its ignored anyway. - m.testConfig = m.fixTestConfig(ctx, m.provider.TestConfig) - - // 3) Write ARCH/Module.apk in testcases. - // Handled by soong_app_prebuilt and OutputFile in entries. - // Nothing to do here. - - // 4) Copy base's data files. - // Handled by soong_app_prebuilt and LOCAL_COMPATIBILITY_SUPPORT_FILES. - // Nothing to do here. + return true } func TestModuleConfigFactory() android.Module { @@ -184,6 +202,16 @@ func TestModuleConfigFactory() android.Module { return module } +func TestModuleConfigHostFactory() android.Module { + module := &testModuleConfigHostModule{} + + module.AddProperties(&module.tradefedProperties) + android.InitAndroidMultiTargetsArchModule(module, android.HostSupported, android.MultilibCommon) + android.InitDefaultableModule(module) + + return module +} + // Implements android.AndroidMkEntriesProvider var _ android.AndroidMkEntriesProvider = (*testModuleConfigModule)(nil) @@ -198,22 +226,96 @@ func (m *testModuleConfigModule) AndroidMkEntries() []android.AndroidMkEntries { // Out update config file with extra options. entries.SetPath("LOCAL_FULL_TEST_CONFIG", m.testConfig) entries.SetString("LOCAL_MODULE_TAGS", "tests") - // Required for atest to run additional tradefed testtypes - entries.AddStrings("LOCAL_HOST_REQUIRED_MODULES", m.provider.HostRequiredModuleNames...) - - // Clear the JNI symbols because they belong to base not us. Either transform the names in the string - // or clear the variable because we don't need it, we are copying bases libraries not generating - // new ones. - entries.SetString("LOCAL_SOONG_JNI_LIBS_SYMBOLS", "") // Don't append to base's test-suites, only use the ones we define, so clear it before // appending to it. entries.SetString("LOCAL_COMPATIBILITY_SUITE", "") - if len(m.tradefedProperties.Test_suites) > 0 { - entries.AddCompatibilityTestSuites(m.tradefedProperties.Test_suites...) - } else { - entries.AddCompatibilityTestSuites("null-suite") + entries.AddCompatibilityTestSuites(m.tradefedProperties.Test_suites...) + + if len(m.provider.HostRequiredModuleNames) > 0 { + entries.AddStrings("LOCAL_HOST_REQUIRED_MODULES", m.provider.HostRequiredModuleNames...) + } + if len(m.provider.RequiredModuleNames) > 0 { + entries.AddStrings("LOCAL_REQUIRED_MODULES", m.provider.RequiredModuleNames...) + } + + if m.provider.IsHost == false { + // Not needed for jar_host_test + // + // Clear the JNI symbols because they belong to base not us. Either transform the names in the string + // or clear the variable because we don't need it, we are copying bases libraries not generating + // new ones. + entries.SetString("LOCAL_SOONG_JNI_LIBS_SYMBOLS", "") } }) return entriesList } + +func (m *testModuleConfigHostModule) DepsMutator(ctx android.BottomUpMutatorContext) { + if m.Base == nil { + ctx.ModuleErrorf("'base' field must be set to a 'java_test_host' module") + return + } + ctx.AddVariationDependencies(ctx.Config().BuildOSCommonTarget.Variations(), testModuleConfigHostTag, *m.Base) +} + +// File to write: +// 1) out/host/linux-x86/testcases/derived-module/test_module_config.manifest # contains base's name. +// 2) out/host/linux-x86/testcases/derived-module/derived-module.config # Update AnroidTest.xml +// 3) out/host/linux-x86/testcases/derived-module/base.jar +// - written via soong_java_prebuilt.mk +// +// 4) out/host/linux-x86/testcases/derived-module/* # data dependencies from base. +// - written via soong_java_prebuilt.mk +func (m *testModuleConfigHostModule) GenerateAndroidBuildActions(ctx android.ModuleContext) { + m.validateBase(ctx, &testModuleConfigHostTag, "java_test_host", true) + m.generateManifestAndConfig(ctx) +} + +// Ensure the base listed is the right type by checking that we get the expected provider data. +// Returns false on errors and the context is updated with an error indicating the baseType expected. +func (m *testModuleConfigModule) validateBase(ctx android.ModuleContext, depTag *dependencyTag, baseType string, baseShouldBeHost bool) { + ctx.VisitDirectDepsWithTag(*depTag, func(dep android.Module) { + if provider, ok := android.OtherModuleProvider(ctx, dep, tradefed.BaseTestProviderKey); ok { + if baseShouldBeHost == provider.IsHost { + m.base = dep + m.provider = provider + } else { + if baseShouldBeHost { + ctx.ModuleErrorf("'android_test' module used as base, but 'java_test_host' expected.") + } else { + ctx.ModuleErrorf("'java_test_host' module used as base, but 'android_test' expected.") + } + } + } else { + ctx.ModuleErrorf("'%s' module used as base but it is not a '%s' module.", *m.Base, baseType) + } + }) +} + +// Actions to write: +// 1. manifest file to testcases dir +// 2. New Module.config / AndroidTest.xml file with our options. +func (m *testModuleConfigModule) generateManifestAndConfig(ctx android.ModuleContext) { + if !m.validateTestSuites(ctx) { + return + } + // Ensure the provider is accurate + if m.provider.TestConfig == nil { + return + } + + // 1) A manifest file listing the base, write text to a tiny file. + installDir := android.PathForModuleInstall(ctx, ctx.ModuleName()) + manifest := android.PathForModuleOut(ctx, "test_module_config.manifest") + android.WriteFileRule(ctx, manifest, fmt.Sprintf("{%q: %q}", "base", *m.tradefedProperties.Base)) + // build/soong/android/androidmk.go has this comment: + // Assume the primary install file is last + // so we need to Install our file last. + ctx.InstallFile(installDir, manifest.Base(), manifest) + + // 2) Module.config / AndroidTest.xml + m.testConfig = m.fixTestConfig(ctx, m.provider.TestConfig) +} + +var _ android.AndroidMkEntriesProvider = (*testModuleConfigHostModule)(nil) diff --git a/tradefed_modules/test_module_config_test.go b/tradefed_modules/test_module_config_test.go index ff5304373..41dd3d479 100644 --- a/tradefed_modules/test_module_config_test.go +++ b/tradefed_modules/test_module_config_test.go @@ -16,6 +16,7 @@ package tradefed_modules import ( "android/soong/android" "android/soong/java" + "strconv" "strings" "testing" ) @@ -43,6 +44,7 @@ const bp = ` base: "base", exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests"], } ` @@ -92,7 +94,7 @@ func TestModuleConfigOptions(t *testing.T) { } // Ensure we error for a base we don't support. -func TestModuleConfigBadBaseShouldFail(t *testing.T) { +func TestModuleConfigWithHostBaseShouldFailWithExplicitMessage(t *testing.T) { badBp := ` java_test_host { name: "base", @@ -104,16 +106,60 @@ func TestModuleConfigBadBaseShouldFail(t *testing.T) { base: "base", exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests"], }` - ctx := android.GroupFixturePreparers( + android.GroupFixturePreparers( java.PrepareForTestWithJavaDefaultModules, android.FixtureRegisterWithContext(RegisterTestModuleConfigBuildComponents), ).ExtendWithErrorHandler( - android.FixtureExpectsAtLeastOneErrorMatchingPattern("does not provide test BaseTestProviderData")). + android.FixtureExpectsAtLeastOneErrorMatchingPattern("'java_test_host' module used as base, but 'android_test' expected")). RunTestWithBp(t, badBp) +} - ctx.ModuleForTests("derived_test", "android_common") +func TestModuleConfigBadBaseShouldFailWithGeneralMessage(t *testing.T) { + badBp := ` + java_library { + name: "base", + srcs: ["a.java"], + } + + test_module_config { + name: "derived_test", + base: "base", + exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], + include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests"], + }` + + android.GroupFixturePreparers( + java.PrepareForTestWithJavaDefaultModules, + android.FixtureRegisterWithContext(RegisterTestModuleConfigBuildComponents), + ).ExtendWithErrorHandler( + android.FixtureExpectsOneErrorPattern("'base' module used as base but it is not a 'android_test' module.")). + RunTestWithBp(t, badBp) +} + +func TestModuleConfigNoBaseShouldFail(t *testing.T) { + badBp := ` + java_library { + name: "base", + srcs: ["a.java"], + } + + test_module_config { + name: "derived_test", + exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], + include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests"], + }` + + android.GroupFixturePreparers( + java.PrepareForTestWithJavaDefaultModules, + android.FixtureRegisterWithContext(RegisterTestModuleConfigBuildComponents), + ).ExtendWithErrorHandler( + android.FixtureExpectsOneErrorPattern("'base' field must be set to a 'android_test' module.")). + RunTestWithBp(t, badBp) } // Ensure we error for a base we don't support. @@ -128,6 +174,7 @@ func TestModuleConfigNoFiltersOrAnnotationsShouldFail(t *testing.T) { test_module_config { name: "derived_test", base: "base", + test_suites: ["general-tests"], }` ctx := android.GroupFixturePreparers( @@ -152,12 +199,14 @@ func TestModuleConfigMultipleDerivedTestsWriteDistinctMakeEntries(t *testing.T) name: "derived_test", base: "base", include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests"], } test_module_config { name: "another_derived_test", base: "base", include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests"], }` ctx := android.GroupFixturePreparers( @@ -190,6 +239,114 @@ func TestModuleConfigMultipleDerivedTestsWriteDistinctMakeEntries(t *testing.T) } } +// Test_module_config_host rule is allowed to depend on java_test_host +func TestModuleConfigHostBasics(t *testing.T) { + bp := ` + java_test_host { + name: "base", + srcs: ["a.java"], + test_suites: ["suiteA", "general-tests", "suiteB"], + } + + test_module_config_host { + name: "derived_test", + base: "base", + exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], + include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests"], + }` + + ctx := android.GroupFixturePreparers( + java.PrepareForTestWithJavaDefaultModules, + android.FixtureRegisterWithContext(RegisterTestModuleConfigBuildComponents), + ).RunTestWithBp(t, bp) + + variant := ctx.Config.BuildOS.String() + "_common" + derived := ctx.ModuleForTests("derived_test", variant) + mod := derived.Module().(*testModuleConfigHostModule) + allEntries := android.AndroidMkEntriesForTest(t, ctx.TestContext, mod) + entries := allEntries[0] + android.AssertArrayString(t, "", entries.EntryMap["LOCAL_MODULE"], []string{"derived_test"}) + + if !mod.Host() { + t.Errorf("host bit is not set for a java_test_host module.") + } + actualData, _ := strconv.ParseBool(entries.EntryMap["LOCAL_IS_UNIT_TEST"][0]) + android.AssertBoolEquals(t, "LOCAL_IS_UNIT_TEST", true, actualData) + +} + +// When you pass an 'android_test' as base, the warning message is a bit obscure, +// talking about variants, but it is something. Ideally we could do better. +func TestModuleConfigHostBadBaseShouldFailWithVariantWarning(t *testing.T) { + badBp := ` + android_test { + name: "base", + sdk_version: "current", + srcs: ["a.java"], + } + + test_module_config_host { + name: "derived_test", + base: "base", + exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], + include_annotations: ["android.platform.test.annotations.LargeTest"], + }` + + android.GroupFixturePreparers( + java.PrepareForTestWithJavaDefaultModules, + android.FixtureRegisterWithContext(RegisterTestModuleConfigBuildComponents), + ).ExtendWithErrorHandler( + android.FixtureExpectsAtLeastOneErrorMatchingPattern("missing variant")). + RunTestWithBp(t, badBp) +} + +func TestModuleConfigHostNeedsATestSuite(t *testing.T) { + badBp := ` + java_test_host { + name: "base", + srcs: ["a.java"], + } + + test_module_config_host { + name: "derived_test", + base: "base", + exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], + include_annotations: ["android.platform.test.annotations.LargeTest"], + }` + + android.GroupFixturePreparers( + java.PrepareForTestWithJavaDefaultModules, + android.FixtureRegisterWithContext(RegisterTestModuleConfigBuildComponents), + ).ExtendWithErrorHandler( + android.FixtureExpectsAtLeastOneErrorMatchingPattern("At least one test-suite must be set")). + RunTestWithBp(t, badBp) +} + +func TestModuleConfigHostDuplicateTestSuitesGiveErrors(t *testing.T) { + badBp := ` + java_test_host { + name: "base", + srcs: ["a.java"], + test_suites: ["general-tests", "some-compat"], + } + + test_module_config_host { + name: "derived_test", + base: "base", + exclude_filters: ["android.test.example.devcodelab.DevCodelabTest#testHelloFail"], + include_annotations: ["android.platform.test.annotations.LargeTest"], + test_suites: ["general-tests", "some-compat"], + }` + + android.GroupFixturePreparers( + java.PrepareForTestWithJavaDefaultModules, + android.FixtureRegisterWithContext(RegisterTestModuleConfigBuildComponents), + ).ExtendWithErrorHandler( + android.FixtureExpectsAtLeastOneErrorMatchingPattern("TestSuite some-compat exists in the base")). + RunTestWithBp(t, badBp) +} + // Use for situations where the entries map contains pairs: [srcPath:installedPath1, srcPath2:installedPath2] // and we want to compare the RHS of the pairs, i.e. installedPath1, installedPath2 func assertEntryPairValues(t *testing.T, actual []string, expected []string) {