More thorough XML testing.

Include vendor.xml in unit tests. Note that changing the build target
with lunch can switch your vendor.xml.

Update DefaultCarrierConfigService#readConfigFromXml to throw errors
instead of returning an empty bundle. This makes failed test output much
better.

Add a test to check that every variable in XML files matches a KEY in
CarrierConfigManager.

Increase detail of error messages with
XmlPullParser#getPositionDescription

Bug: 21619172

Change-Id: I500f4b2476a6fe4fdd6ae0232e293a0f1d82b7b0

2 files changed, 119 insertions, 36 deletions

diff --git a/src/com/android/carrierconfig/DefaultCarrierConfigService.java b/src/com/android/carrierconfig/DefaultCarrierConfigService.java
index 2317156..9081821 100644
--- a/src/com/android/carrierconfig/DefaultCarrierConfigService.java
+++ b/src/com/android/carrierconfig/DefaultCarrierConfigService.java
@@ -58,35 +58,45 @@ public class DefaultCarrierConfigService extends CarrierService {
             return null;
         }
 
-        String fileName = "carrier_config_" + id.getMcc() + id.getMnc() + ".xml";
+
+        PersistableBundle config = null;
         try {
             synchronized (this) {
                 if (mFactory == null) {
                     mFactory = XmlPullParserFactory.newInstance();
                 }
             }
+
             XmlPullParser parser = mFactory.newPullParser();
+            String fileName = "carrier_config_" + id.getMcc() + id.getMnc() + ".xml";
             parser.setInput(getApplicationContext().getAssets().open(fileName), "utf-8");
-            PersistableBundle config = readConfigFromXml(parser, id);
-            // Treat vendor.xml as if it were appended to the carrier config file we read.
-            XmlPullParser vendorInput = getApplicationContext().getResources().getXml(R.xml.vendor);
+            config = readConfigFromXml(parser, id);
+        }
+        catch (IOException | XmlPullParserException e) {
+            Log.d(TAG, e.toString());
+            // We can return an empty config for unknown networks.
+            config = new PersistableBundle();
+        }
+
+        // Treat vendor.xml as if it were appended to the carrier config file we read.
+        XmlPullParser vendorInput = getApplicationContext().getResources().getXml(R.xml.vendor);
+        try {
             PersistableBundle vendorConfig = readConfigFromXml(vendorInput, id);
             config.putAll(vendorConfig);
-            return config;
         }
         catch (IOException | XmlPullParserException e) {
-            // Return null for unknown networks - they should use the defaults.
             Log.e(TAG, e.toString());
-            return null;
         }
+
+        return config;
     }
 
     /**
      * Parses an XML document and returns a PersistableBundle.
      *
-     * <p>This function iterates over each {@code <carrier_config>} node in the XML document and parses
-     * it into a bundle if its filters match {@code id}. The format of XML bundles is defined by
-     * {@link PersistableBundle#restoreFromXml}. All the matching bundles will be flattened and
+     * <p>This function iterates over each {@code <carrier_config>} node in the XML document and
+     * parses it into a bundle if its filters match {@code id}. The format of XML bundles is defined
+     * by {@link PersistableBundle#restoreFromXml}. All the matching bundles will be flattened and
      * returned as a single bundle.</p>
      *
      * <p>Here is an example document. The second bundle will be applied to the first only if the
@@ -106,34 +116,27 @@ public class DefaultCarrierConfigService extends CarrierService {
      * @param id the details of the SIM operator used to filter parts of the document
      * @return a possibly empty PersistableBundle containing the config values.
      */
-    static PersistableBundle readConfigFromXml(XmlPullParser parser, CarrierIdentifier id) {
+    static PersistableBundle readConfigFromXml(XmlPullParser parser, CarrierIdentifier id)
+            throws IOException, XmlPullParserException {
         PersistableBundle config = new PersistableBundle();
 
         if (parser == null) {
           return config;
         }
 
-        try {
-            // Iterate over each <carrier_config> node in the document and add it to the returned
-            // bundle if its filters match.
-            int event;
-            while (((event = parser.next()) != XmlPullParser.END_DOCUMENT)) {
-                if (event == XmlPullParser.START_TAG && "carrier_config".equals(parser.getName())) {
-                    // Skip this fragment if it has filters that don't match.
-                    if (!checkFilters(parser, id)) {
-                        continue;
-                    }
-                    PersistableBundle configFragment = PersistableBundle.restoreFromXml(parser);
-                    config.putAll(configFragment);
+        // Iterate over each <carrier_config> node in the document and add it to the returned
+        // bundle if its filters match.
+        int event;
+        while (((event = parser.next()) != XmlPullParser.END_DOCUMENT)) {
+            if (event == XmlPullParser.START_TAG && "carrier_config".equals(parser.getName())) {
+                // Skip this fragment if it has filters that don't match.
+                if (!checkFilters(parser, id)) {
+                    continue;
                 }
+                PersistableBundle configFragment = PersistableBundle.restoreFromXml(parser);
+                config.putAll(configFragment);
             }
         }
-        catch (XmlPullParserException e) {
-            Log.e(TAG, e.toString());
-        }
-        catch (IOException e) {
-            Log.e(TAG, e.toString());
-        }
 
         return config;
     }
@@ -141,10 +144,10 @@ public class DefaultCarrierConfigService extends CarrierService {
     /**
      * Checks to see if an XML node matches carrier filters.
      *
-     * <p>This iterates over the attributes of the current tag pointed to by {@code parser} and checks
-     * each one against {@code id} or {@link Build.DEVICE}. Attributes that are not specified in the
-     * node will not be checked, so a node with no attributes will always return true. The supported
-     * filter attributes are,
+     * <p>This iterates over the attributes of the current tag pointed to by {@code parser} and
+     * checks each one against {@code id} or {@link Build.DEVICE}. Attributes that are not specified
+     * in the node will not be checked, so a node with no attributes will always return true. The
+     * supported filter attributes are,
      * <ul>
      *   <li>mcc: {@link CarrierIdentifier#getMcc}</li>
      *   <li>mnc: {@link CarrierIdentifier#getMnc}</li>
diff --git a/tests/src/com/android/carrierconfig/CarrierConfigTest.java b/tests/src/com/android/carrierconfig/CarrierConfigTest.java
index 5a29ea1..58802f3 100644
--- a/tests/src/com/android/carrierconfig/CarrierConfigTest.java
+++ b/tests/src/com/android/carrierconfig/CarrierConfigTest.java
@@ -2,13 +2,19 @@ package com.android.carrierconfig;
 
 import android.content.Context;
 import android.content.res.AssetManager;
+import android.content.res.Resources;
 import android.os.PersistableBundle;
 import android.service.carrier.CarrierIdentifier;
+import android.telephony.CarrierConfigManager;
 import android.test.InstrumentationTestCase;
 import android.util.Log;
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.HashSet;
+import java.util.Set;
 
 import junit.framework.AssertionFailedError;
 
@@ -27,7 +33,6 @@ public class CarrierConfigTest extends InstrumentationTestCase {
                 PersistableBundle b = DefaultCarrierConfigService.readConfigFromXml(parser,
                         new CarrierIdentifier("001", "001", "Test", "", "", ""));
                 assertNotNull("got null bundle", b);
-                assertTrue("got empty bundle", b.size() > 0);
             }
         });
     }
@@ -41,7 +46,8 @@ public class CarrierConfigTest extends InstrumentationTestCase {
             public void check(XmlPullParser parser) throws XmlPullParserException, IOException {
                 int event;
                 while (((event = parser.next()) != XmlPullParser.END_DOCUMENT)) {
-                    if (event == XmlPullParser.START_TAG && "carrier_config".equals(parser.getName())) {
+                    if (event == XmlPullParser.START_TAG
+                            && "carrier_config".equals(parser.getName())) {
                         for (int i = 0; i < parser.getAttributeCount(); ++i) {
                             String attribute = parser.getAttributeName(i);
                             switch (attribute) {
@@ -53,7 +59,8 @@ public class CarrierConfigTest extends InstrumentationTestCase {
                                 case "device":
                                     break;
                                 default:
-                                    fail("Unknown attribute '" + attribute  + "'");
+                                    fail("Unknown attribute '" + attribute
+                                            + "' at " + parser.getPositionDescription());
                                     break;
                             }
                         }
@@ -64,6 +71,48 @@ public class CarrierConfigTest extends InstrumentationTestCase {
     }
 
     /**
+     * Tests that the variable names in each XML file match actual keys in CarrierConfigManager.
+     */
+    public void testVariableNames() {
+        final Set<String> varXmlNames = getCarrierConfigXmlNames();
+        // organize them into sets by type or unknown
+        forEachConfigXml(new ParserChecker() {
+            public void check(XmlPullParser parser) throws XmlPullParserException, IOException {
+                int event;
+                while (((event = parser.next()) != XmlPullParser.END_DOCUMENT)) {
+                    if (event == XmlPullParser.START_TAG) {
+                        switch (parser.getName()) {
+                            case "int":
+                            case "boolean":
+                            case "string":
+                            case "int-array":
+                            case "string-array":
+                                // NOTE: This doesn't check for other valid Bundle values, but it
+                                // is limited to the key types in CarrierConfigManager.
+                                final String varName = parser.getAttributeValue(null, "name");
+                                assertNotNull("No 'name' attribute: "
+                                        + parser.getPositionDescription(), varName);
+                                assertTrue("Unknown variable: '" + varName
+                                        + "' at " + parser.getPositionDescription(),
+                                        varXmlNames.contains(varName));
+                                // TODO: Check that the type is correct.
+                                break;
+                            case "carrier_config_list":
+                            case "carrier_config":
+                                // do nothing
+                                break;
+                            default:
+                                fail("unexpected tag: '" + parser.getName()
+                                        + "' at " + parser.getPositionDescription());
+                                break;
+                        }
+                    }
+                }
+            }
+        });
+    }
+
+    /**
      * Utility for iterating over each XML document in the assets folder.
      *
      * This can be used with {@link #forEachConfigXml} to run checks on each XML document.
@@ -96,8 +145,39 @@ public class CarrierConfigTest extends InstrumentationTestCase {
                     throw new AssertionError("Problem in " + fileName + ": " + e.getMessage(), e);
                 }
             }
+            // Check vendor.xml too
+            try {
+                Resources res = getInstrumentation().getTargetContext().getResources();
+                checker.check(res.getXml(R.xml.vendor));
+            } catch (Throwable e) {
+                throw new AssertionError("Problem in vendor.xml: " + e.getMessage(), e);
+            }
         } catch (IOException e) {
             fail(e.toString());
         }
     }
+
+    /**
+     * Get the set of config variable names, as used in XML files.
+     */
+    private Set<String> getCarrierConfigXmlNames() {
+        // get values of all KEY_ members of CarrierConfigManager
+        Field[] fields = CarrierConfigManager.class.getDeclaredFields();
+        HashSet<String> varXmlNames = new HashSet<>();
+        for (Field f : fields) {
+            if (!f.getName().startsWith("KEY_")) continue;
+            if ((f.getModifiers() & Modifier.STATIC) == 0) {
+                fail("non-static key in CarrierConfigManager: " + f.toString());
+            }
+            try {
+                String value = (String) f.get(null);
+                varXmlNames.add(value);
+            }
+            catch (IllegalAccessException e) {
+                throw new AssertionError("Failed to get config key: " + e.getMessage(), e);
+            }
+        }
+        assertTrue("Found zero keys", varXmlNames.size() > 0);
+        return varXmlNames;
+    }
 }