Merge "Fix interleaved device config situation" into oc-dev

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);
     }
 
     /**