test(parser): Default parser

[msohailhussain]

Summary

Modified travis file to test all unit tests with default config parsers and default config parser is now set using ENV.
All Parsers are defined in matrix to run all unit tests with each parser.

Test plan

Unit tests should be pased

Issues

• "THING-1234" or "Fixes parameterize optimizely unit tests with v4 datafile #123"

[coveralls]

Pull Request Test Coverage Report for Build 1120

• 19 of 32 (59.38%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.06%) to 89.66%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/parser/DefaultConfigParser.java	19	32	59.38%

Totals
Change from base Build 1111:	-0.06%
Covered Lines:	3425
Relevant Lines:	3820

---

💛 - Coveralls

[mikecdavis]

I think we should be leveraging the enum class better to remove the redundant mappings etc that we have now. I've provided some code samples as guidance.

[mikecdavis]

With the enum above and by assuming the configuration override is using the enum this can be cleaned up to:

        String configParserName = PropertyUtils.get("default_parser");
        if (configParserName != null) {
            try {
                ConfigParserSupplier supplier = ConfigParserSupplier.valueOf(configParserName);
                if (supplier.isPresent()) {
                    ConfigParser configParser = supplier.get();
                    logger.debug("using json parser: {}, based on override config", configParser.getClass().getSimpleName());
                    return configParser;
                }

                logger.warn("configured parser {} is not available in the classpath", configParserName);
            } catch (IllegalArgumentException e) {
                logger.warn("configured parser {} is not a valid value", configParserName);
            }
        }

[mikecdavis]

We should use the enums here to make it clear there are a set number of supported parsers.

[mikecdavis]

Let's use a supplier in the constructor and change the enum name:

    public enum ConfigParserSupplier {
        GSON_CONFIG_PARSER("com.google.gson.Gson", GsonConfigParser::new),
        JACKSON_CONFIG_PARSER("com.fasterxml.jackson.databind.ObjectMapper", JacksonConfigParser::new),
        JSON_CONFIG_PARSER("org.json.JSONObject", JsonConfigParser::new),
        JSON_SIMPLE_CONFIG_PARSER("org.json.simple.JSONObject", JsonSimpleConfigParser::new);

        private final String className;
        private final Supplier<ConfigParser> supplier;

        ConfigParserSupplier(String className, Supplier<ConfigParser> supplier) {
            this.className = className;
            this.supplier = supplier;
        }

        ConfigParser get() {
            return supplier.get();
        }

        private boolean isPresent() {
            try {
                Class.forName(className);
                return true;
            } catch (ClassNotFoundException e) {
                return false;
            }
        }
    }

Note that I moved isPresent into this enum, but not a strict requirement.

[mikecdavis]

Now that we have the enum as defined above this changes from if...else if..., to:

        for (ConfigParserSupplier supplier: ConfigParserSupplier.valuesI()) {
            if (!supplier.isPresent()) {
                continue;
            }

            ConfigParser configParser = supplier.get();
            logger.info("using json parser: {}", configParser.getClass().getSimpleName());
            return configParser;
        }

        throw new MissingJsonParserException("unable to locate a JSON parser. "
            + "Please see <link> for more information");

[mikecdavis]

with the enum as defined earlier we can use ConfigParserSupplier.valueOf(defaultParser).

[mikecdavis]

can we have an else statement defaulting to the first enum that matches? not sure if this will be deterministic however, but worth a shot.

[mnoman09]

yes, we can do that first enum that matches will be GSON_CONFIG_PARSER

[mikecdavis]

One last change in the test case.

[mikecdavis]

Should be a switch statement.

Class expectedParser = GsonConfigParser.class;

if(defaultParser != null) {
    DefaultConfigParser.ConfigParserSupplier defaultParserSupplier = DefaultConfigParser.ConfigParserSupplier.valueOf(defaultParser);
    switch (defaultParserSupplier) {
        case GSON_CONFIG_PARSER:
            expectedParser = GsonConfigParser.class;
            break;
        case JACKSON_CONFIG_PARSER:
            expectedParser = JacksonConfigParser.class;
            break;
        case JSON_CONFIG_PARSER:
            expectedParser = JsonConfigParser.class;
            break;
        case JSON_SIMPLE_CONFIG_PARSER:
            expectedParser = JsonSimpleConfigParser.class;
            break;
        default:
            fail("Not a valid config parser");
    }
}

Assert.assertThat(configParser, CoreMatchers.instanceOf(expectedParser));