diff options
author | jdesprez <jdesprez@google.com> | 2017-04-24 15:56:48 -0700 |
---|---|---|
committer | Julien Desprez <jdesprez@google.com> | 2017-06-16 21:58:35 +0000 |
commit | 38d983b4bb46495da83831e13362be925fe757d9 (patch) | |
tree | 87a93854fd8a3502db5b9a890872c1d09cb6f6ed | |
parent | 504a704fbfc1bdf68a1d7eb59e951d415bfe68e9 (diff) | |
download | tradefederation-38d983b4bb46495da83831e13362be925fe757d9.tar.gz |
Fix interleaved device config situation
If device config are interleaved and define same object
with options value, the targetting per appearance number
will fail since we are re-ordering the device together.
- Carry over the initial appearance number to be used when
assigning the option values.
Test: unit tests, local run
Bug: 36101693
Change-Id: I1b8a4679b2d4a1d221ea636624a7c7382bdf92ff
(cherry picked from commit 430902d78c5316a311b4a645e8b3477b5788ffc6)
9 files changed, 243 insertions, 24 deletions
diff --git a/src/com/android/tradefed/config/ConfigurationDef.java b/src/com/android/tradefed/config/ConfigurationDef.java index f46fc853c..7f96298b1 100644 --- a/src/com/android/tradefed/config/ConfigurationDef.java +++ b/src/com/android/tradefed/config/ConfigurationDef.java @@ -36,7 +36,7 @@ public class ConfigurationDef { * a map of object type names to config object class name(s). Use LinkedHashMap to keep objects * in the same order they were added. */ - private final Map<String, List<String>> mObjectClassMap = new LinkedHashMap<>(); + private final Map<String, List<ConfigObjectDef>> mObjectClassMap = new LinkedHashMap<>(); /** a list of option name/value pairs. */ private final List<OptionDef> mOptionList = new ArrayList<>(); @@ -65,6 +65,20 @@ public class ConfigurationDef { } } + /** + * Object to hold info for a className and the appearance number it has (e.g. if a config has + * the same object twice, the first one will have the first appearance number). + */ + public static class ConfigObjectDef { + final String mClassName; + final Integer mAppearanceNum; + + ConfigObjectDef(String className, Integer appearance) { + mClassName = className; + mAppearanceNum = appearance; + } + } + private boolean mMultiDeviceMode = false; private Set<String> mExpectedDevices = new HashSet<>(); private static final Pattern MULTI_PATTERN = Pattern.compile("(.*)(:)(.*)"); @@ -105,17 +119,17 @@ public class ConfigurationDef { * just-added instance of <code>clasName</code>. */ int addConfigObjectDef(String typeName, String className) { - List<String> classList = mObjectClassMap.get(typeName); + List<ConfigObjectDef> classList = mObjectClassMap.get(typeName); if (classList == null) { - classList = new ArrayList<String>(); + classList = new ArrayList<ConfigObjectDef>(); mObjectClassMap.put(typeName, classList); } - classList.add(className); // Increment and store count for this className Integer freq = mClassFrequency.get(className); freq = freq == null ? 1 : freq + 1; mClassFrequency.put(className, freq); + classList.add(new ConfigObjectDef(className, freq)); return freq; } @@ -153,10 +167,10 @@ public class ConfigurationDef { /** * Get the object type name-class map. - * <p/> - * Exposed for unit testing + * + * <p>Exposed for unit testing */ - Map<String, List<String>> getObjectClassMap() { + Map<String, List<ConfigObjectDef>> getObjectClassMap() { return mObjectClassMap; } @@ -191,12 +205,13 @@ public class ConfigurationDef { } } - for (Map.Entry<String, List<String>> objClassEntry : mObjectClassMap.entrySet()) { + for (Map.Entry<String, List<ConfigObjectDef>> objClassEntry : mObjectClassMap.entrySet()) { List<Object> objectList = new ArrayList<Object>(objClassEntry.getValue().size()); String entryName = objClassEntry.getKey(); boolean shouldAddToFlatConfig = true; - for (String className : objClassEntry.getValue()) { - Object configObject = createObject(objClassEntry.getKey(), className); + + for (ConfigObjectDef configDef : objClassEntry.getValue()) { + Object configObject = createObject(objClassEntry.getKey(), configDef.mClassName); Matcher matcher = null; if (mMultiDeviceMode) { matcher = MULTI_PATTERN.matcher(entryName); @@ -218,9 +233,11 @@ public class ConfigurationDef { } // We reference the original object to the device and not to the flat list. multiDev.addSpecificConfig(configObject); + multiDev.addFrequency(configObject, configDef.mAppearanceNum); } else { if (Configuration.doesBuiltInObjSupportMultiDevice(entryName)) { defaultDeviceConfig.addSpecificConfig(configObject); + defaultDeviceConfig.addFrequency(configObject, configDef.mAppearanceNum); } else { // Only add to flat list if they are not part of multi device config. objectList.add(configObject); @@ -248,10 +265,10 @@ public class ConfigurationDef { IGlobalConfiguration createGlobalConfiguration() throws ConfigurationException { IGlobalConfiguration config = new GlobalConfiguration(getName(), getDescription()); - for (Map.Entry<String, List<String>> objClassEntry : mObjectClassMap.entrySet()) { + for (Map.Entry<String, List<ConfigObjectDef>> objClassEntry : mObjectClassMap.entrySet()) { List<Object> objectList = new ArrayList<Object>(objClassEntry.getValue().size()); - for (String className : objClassEntry.getValue()) { - Object configObject = createObject(objClassEntry.getKey(), className); + for (ConfigObjectDef configDef : objClassEntry.getValue()) { + Object configObject = createObject(objClassEntry.getKey(), configDef.mClassName); objectList.add(configObject); } config.setConfigurationObjectList(objClassEntry.getKey(), objectList); diff --git a/src/com/android/tradefed/config/DeviceConfigurationHolder.java b/src/com/android/tradefed/config/DeviceConfigurationHolder.java index eaec80f99..cf4c59403 100644 --- a/src/com/android/tradefed/config/DeviceConfigurationHolder.java +++ b/src/com/android/tradefed/config/DeviceConfigurationHolder.java @@ -26,7 +26,9 @@ import com.android.tradefed.log.LogUtil.CLog; import com.android.tradefed.targetprep.ITargetPreparer; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * A concrete {@link IDeviceConfiguration} implementation that stores the loaded device @@ -40,6 +42,8 @@ public class DeviceConfigurationHolder implements IDeviceConfiguration { private IDeviceSelection mDeviceSelection = new DeviceSelectionOptions(); private TestDeviceOptions mTestDeviceOption = new TestDeviceOptions(); + private Map<Object, Integer> mFreqMap = new HashMap<>(); + public DeviceConfigurationHolder() { mDeviceName = ""; } @@ -77,9 +81,19 @@ public class DeviceConfigurationHolder implements IDeviceConfiguration { } } - /** - * {@inheritDoc} - */ + /** {@inheritDoc} */ + @Override + public void addFrequency(Object config, Integer frequency) { + mFreqMap.put(config, frequency); + } + + /** {@inheritDoc} */ + @Override + public Integer getFrequency(Object config) { + return mFreqMap.get(config); + } + + /** {@inheritDoc} */ @Override public List<Object> getAllObjects() { List<Object> allObject = new ArrayList<Object>(); diff --git a/src/com/android/tradefed/config/IDeviceConfiguration.java b/src/com/android/tradefed/config/IDeviceConfiguration.java index d0fc2c811..53e4726f8 100644 --- a/src/com/android/tradefed/config/IDeviceConfiguration.java +++ b/src/com/android/tradefed/config/IDeviceConfiguration.java @@ -52,8 +52,17 @@ public interface IDeviceConfiguration { public void addSpecificConfig(Object config) throws ConfigurationException; /** - * Return {@link IBuildProvider} that the device configuration holder has reference to. + * Keep track of the frequency of the object so we can properly inject option against it. + * + * @param config the object we are tracking the frequency. + * @param frequency frequency associated with the object. */ + public void addFrequency(Object config, Integer frequency); + + /** Returns the frequency of the object. */ + public Integer getFrequency(Object config); + + /** Return {@link IBuildProvider} that the device configuration holder has reference to. */ public IBuildProvider getBuildProvider(); /** diff --git a/src/com/android/tradefed/config/OptionSetter.java b/src/com/android/tradefed/config/OptionSetter.java index c449f1285..b4eecc9a3 100644 --- a/src/com/android/tradefed/config/OptionSetter.java +++ b/src/com/android/tradefed/config/OptionSetter.java @@ -567,6 +567,11 @@ public class OptionSetter { index = freqMap.get(deviceObject.getClass().getName()); index = index == null ? 1 : index + 1; freqMap.put(deviceObject.getClass().getName(), index); + Integer tracked = + ((IDeviceConfiguration) objectSource).getFrequency(deviceObject); + if (tracked != null && !index.equals(tracked)) { + index = tracked; + } addOptionsForObject(deviceObject, optionMap, index, ((IDeviceConfiguration)objectSource).getDeviceName()); } diff --git a/src/com/android/tradefed/targetprep/DeviceWiper.java b/src/com/android/tradefed/targetprep/DeviceWiper.java index 39957f649..9b828164e 100644 --- a/src/com/android/tradefed/targetprep/DeviceWiper.java +++ b/src/com/android/tradefed/targetprep/DeviceWiper.java @@ -38,6 +38,13 @@ public class DeviceWiper implements ITargetPreparer { "instruct wiper to use fastboot erase instead of format") protected boolean mUseErase = false; + /** + * Return True if this target preparer has been disabled and will do nothing. False otherwise. + */ + public boolean isDisabled() { + return mDisable; + } + @Override public void setUp(ITestDevice device, IBuildInfo buildInfo) throws TargetSetupError, DeviceNotAvailableException { diff --git a/tests/res/testconfigs/multi-device-mix-options.xml b/tests/res/testconfigs/multi-device-mix-options.xml new file mode 100644 index 000000000..6f5ea2bcf --- /dev/null +++ b/tests/res/testconfigs/multi-device-mix-options.xml @@ -0,0 +1,45 @@ +<?xml version="1.0" encoding="utf-8"?> +<!-- Copyright (C) 2017 The Android Open Source Project + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<configuration description="Multi device parsing"> + <device name="device1"> + <build_provider class="com.android.tradefed.build.StubBuildProvider"> + <option name="build-id" value="10" /> + </build_provider> + <target_preparer class="com.android.tradefed.targetprep.DeviceWiper" /> + <!-- First appearance disable it for all of device1 --> + <option name="disable" value="true" /> + </device> + + <device name="device2" > + <build_provider class="com.android.tradefed.build.StubBuildProvider" /> + <target_preparer class="com.android.tradefed.targetprep.DeviceWiper"> + <option name="disable" value="true" /> + </target_preparer> + </device> + + <device name="device1" > + <target_preparer class="com.android.tradefed.targetprep.StubTargetPreparer" /> + <target_preparer class="com.android.tradefed.targetprep.DeviceWiper"> + <!-- second appearance disable it only for itself --> + <option name="disable" value="false" /> + </target_preparer> + </device> + + <logger class="com.android.tradefed.log.FileLogger" /> + <test class="com.android.tradefed.config.StubOptionTest"> + <option name="option" value="valueFromTestConfig" /> + </test> +</configuration> diff --git a/tests/res/testconfigs/multi-device-mix.xml b/tests/res/testconfigs/multi-device-mix.xml new file mode 100644 index 000000000..3f660e663 --- /dev/null +++ b/tests/res/testconfigs/multi-device-mix.xml @@ -0,0 +1,44 @@ +<?xml version="1.0" encoding="utf-8"?> +<!-- Copyright (C) 2017 The Android Open Source Project + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<configuration description="Multi device parsing"> + <device name="device1"> + <build_provider class="com.android.tradefed.build.StubBuildProvider"> + <option name="build-id" value="10" /> + </build_provider> + <target_preparer class="com.android.tradefed.targetprep.DeviceWiper"> + <option name="disable" value="true" /> + </target_preparer> + </device> + + <device name="device2" > + <build_provider class="com.android.tradefed.build.StubBuildProvider" /> + <target_preparer class="com.android.tradefed.targetprep.DeviceWiper"> + <option name="disable" value="true" /> + </target_preparer> + </device> + + <device name="device1" > + <target_preparer class="com.android.tradefed.targetprep.StubTargetPreparer" /> + <target_preparer class="com.android.tradefed.targetprep.DeviceWiper"> + <option name="disable" value="false" /> + </target_preparer> + </device> + + <logger class="com.android.tradefed.log.FileLogger" /> + <test class="com.android.tradefed.config.StubOptionTest"> + <option name="option" value="valueFromTestConfig" /> + </test> +</configuration> diff --git a/tests/src/com/android/tradefed/config/ConfigurationFactoryTest.java b/tests/src/com/android/tradefed/config/ConfigurationFactoryTest.java index e3e2e9d3e..54ee9930c 100644 --- a/tests/src/com/android/tradefed/config/ConfigurationFactoryTest.java +++ b/tests/src/com/android/tradefed/config/ConfigurationFactoryTest.java @@ -19,6 +19,7 @@ import com.android.ddmlib.Log.LogLevel; import com.android.tradefed.config.ConfigurationFactory.ConfigId; import com.android.tradefed.log.ILeveledLogOutput; import com.android.tradefed.log.LogUtil.CLog; +import com.android.tradefed.targetprep.DeviceWiper; import com.android.tradefed.targetprep.StubTargetPreparer; import com.android.tradefed.util.FileUtil; @@ -1104,8 +1105,75 @@ public class ConfigurationFactoryTest extends TestCase { } /** - * Configuration for multi device is wrong since it contains a build_provider tag outside - * the devices tags. + * Test that when <device> tags are out of order (device 1 - device 2 - device 1) and an option + * is specified in the last device 1 with an increased frequency (a same class object from the + * first device 1 or 2), the option is properly found and assigned. + */ + public void testCreateConfigurationFromArgs_frequency() throws Exception { + IConfiguration config = + mFactory.createConfigurationFromArgs(new String[] {"multi-device-mix"}); + assertNotNull(config.getDeviceConfigByName("device1")); + assertEquals(3, config.getDeviceConfigByName("device1").getTargetPreparers().size()); + assertTrue( + config.getDeviceConfigByName("device1").getTargetPreparers().get(0) + instanceof DeviceWiper); + DeviceWiper prep1 = + (DeviceWiper) config.getDeviceConfigByName("device1").getTargetPreparers().get(0); + assertTrue(prep1.isDisabled()); + assertTrue( + config.getDeviceConfigByName("device1").getTargetPreparers().get(2) + instanceof DeviceWiper); + DeviceWiper prep3 = + (DeviceWiper) config.getDeviceConfigByName("device1").getTargetPreparers().get(2); + assertFalse(prep3.isDisabled()); + + assertNotNull(config.getDeviceConfigByName("device2")); + assertEquals(1, config.getDeviceConfigByName("device2").getTargetPreparers().size()); + assertTrue( + config.getDeviceConfigByName("device2").getTargetPreparers().get(0) + instanceof DeviceWiper); + DeviceWiper prep2 = + (DeviceWiper) config.getDeviceConfigByName("device2").getTargetPreparers().get(0); + // Only device 1 preparer has been targeted. + assertTrue(prep2.isDisabled()); + } + + /** + * Tests a different usage of options for a multi device interleaved config where options are + * specified. + */ + public void testCreateConfigurationFromArgs_frequency_withOptionOpen() throws Exception { + IConfiguration config = + mFactory.createConfigurationFromArgs(new String[] {"multi-device-mix-options"}); + assertNotNull(config.getDeviceConfigByName("device1")); + assertEquals(3, config.getDeviceConfigByName("device1").getTargetPreparers().size()); + assertTrue( + config.getDeviceConfigByName("device1").getTargetPreparers().get(0) + instanceof DeviceWiper); + DeviceWiper prep1 = + (DeviceWiper) config.getDeviceConfigByName("device1").getTargetPreparers().get(0); + assertTrue(prep1.isDisabled()); + assertTrue( + config.getDeviceConfigByName("device1").getTargetPreparers().get(2) + instanceof DeviceWiper); + DeviceWiper prep3 = + (DeviceWiper) config.getDeviceConfigByName("device1").getTargetPreparers().get(2); + assertFalse(prep3.isDisabled()); + + assertNotNull(config.getDeviceConfigByName("device2")); + assertEquals(1, config.getDeviceConfigByName("device2").getTargetPreparers().size()); + assertTrue( + config.getDeviceConfigByName("device2").getTargetPreparers().get(0) + instanceof DeviceWiper); + DeviceWiper prep2 = + (DeviceWiper) config.getDeviceConfigByName("device2").getTargetPreparers().get(0); + // Only device 1 preparer has been targeted. + assertTrue(prep2.isDisabled()); + } + + /** + * Configuration for multi device is wrong since it contains a build_provider tag outside the + * devices tags. */ public void testCreateConfigurationFromArgs_multidevice_exception() throws Exception { String expectedException = "Tags [build_provider] should be included in a <device> tag."; diff --git a/tests/src/com/android/tradefed/config/ConfigurationXmlParserTest.java b/tests/src/com/android/tradefed/config/ConfigurationXmlParserTest.java index 8cea6db21..5f24da7b4 100644 --- a/tests/src/com/android/tradefed/config/ConfigurationXmlParserTest.java +++ b/tests/src/com/android/tradefed/config/ConfigurationXmlParserTest.java @@ -55,7 +55,9 @@ public class ConfigurationXmlParserTest extends TestCase { xmlParser.parse(configDef, configName, getStringAsStream(normalConfig), null); assertEquals(configName, configDef.getName()); assertEquals("desc", configDef.getDescription()); - assertEquals("junit.framework.TestCase", configDef.getObjectClassMap().get("test").get(0)); + assertEquals( + "junit.framework.TestCase", + configDef.getObjectClassMap().get("test").get(0).mClassName); assertEquals("junit.framework.TestCase:1:opName", configDef.getOptionList().get(0).name); assertEquals("val", configDef.getOptionList().get(0).value); } @@ -75,7 +77,9 @@ public class ConfigurationXmlParserTest extends TestCase { xmlParser.parse(configDef, configName, getStringAsStream(normalConfig), null); assertEquals(configName, configDef.getName()); assertEquals("desc", configDef.getDescription()); - assertEquals("junit.framework.TestCase", configDef.getObjectClassMap().get("test").get(0)); + assertEquals( + "junit.framework.TestCase", + configDef.getObjectClassMap().get("test").get(0).mClassName); // the non-namespaced option value should be used assertEquals("opName", configDef.getOptionList().get(0).name); assertEquals("val", configDef.getOptionList().get(0).value); @@ -101,11 +105,15 @@ public class ConfigurationXmlParserTest extends TestCase { assertEquals(configName, configDef.getName()); assertEquals("desc", configDef.getDescription()); - assertEquals("com.android.tradefed.testtype.HostTest", configDef.getObjectClassMap().get("test").get(0)); + assertEquals( + "com.android.tradefed.testtype.HostTest", + configDef.getObjectClassMap().get("test").get(0).mClassName); assertEquals("com.android.tradefed.testtype.HostTest:1:class", configDef.getOptionList().get(0).name); assertEquals("val1", configDef.getOptionList().get(0).value); - assertEquals("com.android.tradefed.testtype.HostTest", configDef.getObjectClassMap().get("test").get(1)); + assertEquals( + "com.android.tradefed.testtype.HostTest", + configDef.getObjectClassMap().get("test").get(1).mClassName); assertEquals("com.android.tradefed.testtype.HostTest:2:class", configDef.getOptionList().get(1).name); assertEquals("val2", configDef.getOptionList().get(1).value); } @@ -146,7 +154,9 @@ public class ConfigurationXmlParserTest extends TestCase { "<object type=\"foo\" class=\"junit.framework.TestCase\" />"; ConfigurationDef configDef = new ConfigurationDef("name"); xmlParser.parse(configDef, "name", getStringAsStream(config), null); - assertEquals("junit.framework.TestCase", configDef.getObjectClassMap().get("foo").get(0)); + assertEquals( + "junit.framework.TestCase", + configDef.getObjectClassMap().get("foo").get(0).mClassName); } /** |