From: Teemu Murtola Date: Sat, 11 Mar 2017 13:14:30 +0000 (+0200) Subject: Improve option support & tests for key-value trees X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?a=commitdiff_plain;h=a6d9b79c30848065328d7c7a688e80b244035817;p=alexxy%2Fgromacs.git Improve option support & tests for key-value trees Add basic tests for the behavior of different option types with key-value tree handling. For now, the test covers only the "adjust" function. Make enum options serialize as strings instead of integers, which improves behavior of gmx dump and gmx check, and can also be more robust for backward/forward compatibility. Improve the way the key-value tree serialization tests are done: use the in-memory serializer to test roundtrips, instead of relying on the reference data. There is less code this way, and the tests behave better in the absence of reference data. Change-Id: I5e52265333f397bd6e058636691cbfd21092df0d --- diff --git a/src/gromacs/options/basicoptions.cpp b/src/gromacs/options/basicoptions.cpp index 86022b3d75..31f9ceb7b8 100644 --- a/src/gromacs/options/basicoptions.cpp +++ b/src/gromacs/options/basicoptions.cpp @@ -610,6 +610,11 @@ std::string EnumOptionStorage::formatSingleValue(const int &value) const return allowed_[value]; } +Variant EnumOptionStorage::normalizeValue(const int &value) const +{ + return Variant::create(formatSingleValue(value)); +} + void EnumOptionStorage::initConverter(ConverterType *converter) { converter->addConverter( diff --git a/src/gromacs/options/basicoptionstorage.h b/src/gromacs/options/basicoptionstorage.h index 92aefa31fa..f3e4be1fac 100644 --- a/src/gromacs/options/basicoptionstorage.h +++ b/src/gromacs/options/basicoptionstorage.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2011,2012,2013,2014,2015,2016, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014,2015,2016,2017, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -244,6 +244,7 @@ class EnumOptionStorage : public OptionStorageTemplateSimple virtual std::string typeString() const { return "enum"; } virtual std::string formatExtraDescription() const; virtual std::string formatSingleValue(const int &value) const; + virtual Variant normalizeValue(const int &value) const; //! \copydoc EnumOptionInfo::allowedValues() const std::vector &allowedValues() const { return allowed_; } diff --git a/src/gromacs/options/optionstoragetemplate.h b/src/gromacs/options/optionstoragetemplate.h index 2dae1aa9f4..d2d9dac15f 100644 --- a/src/gromacs/options/optionstoragetemplate.h +++ b/src/gromacs/options/optionstoragetemplate.h @@ -334,9 +334,7 @@ class OptionStorageTemplateSimple : public OptionStorageTemplate std::vector result; for (const auto &value : values) { - result.push_back( - Variant::create( - processValue(converter_.convert(value)))); + result.push_back(normalizeValue(converter_.convert(value))); } return result; } @@ -359,6 +357,19 @@ class OptionStorageTemplateSimple : public OptionStorageTemplate { return value; } + /*! \brief + * Converts a single value to normalized type. + * + * \param[in] value Value after conversion. + * \returns Value to store for the option. + * + * This can be overridden to serialize a different type than `T` + * when using the option with KeyValueTreeObject. + */ + virtual Variant normalizeValue(const T &value) const + { + return Variant::create(processValue(value)); + } private: virtual void convertValue(const Variant &variant) @@ -471,7 +482,7 @@ std::vector OptionStorageTemplate::defaultValues() const { result.push_back(Variant::create(value)); } - return result; + return normalizeValues(result); } diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsBooleanOption.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsBooleanOption.xml new file mode 100644 index 0000000000..ae2a0b96ea --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsBooleanOption.xml @@ -0,0 +1,8 @@ + + + + + + true + + diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsDoubleOption.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsDoubleOption.xml new file mode 100644 index 0000000000..03e296ee08 --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsDoubleOption.xml @@ -0,0 +1,8 @@ + + + + + + 1.5 + + diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsEnumIntOption.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsEnumIntOption.xml new file mode 100644 index 0000000000..db293136ae --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsEnumIntOption.xml @@ -0,0 +1,8 @@ + + + + + + foo + + diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsEnumOption.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsEnumOption.xml new file mode 100644 index 0000000000..db293136ae --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsEnumOption.xml @@ -0,0 +1,8 @@ + + + + + + foo + + diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsFloatOption.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsFloatOption.xml new file mode 100644 index 0000000000..03e296ee08 --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsFloatOption.xml @@ -0,0 +1,8 @@ + + + + + + 1.5 + + diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsInt64Option.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsInt64Option.xml new file mode 100644 index 0000000000..d8e09c7166 --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsInt64Option.xml @@ -0,0 +1,8 @@ + + + + + + 2 + + diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsIntegerOption.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsIntegerOption.xml new file mode 100644 index 0000000000..bed5abe4f3 --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsIntegerOption.xml @@ -0,0 +1,8 @@ + + + + + + 2 + + diff --git a/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsStringOption.xml b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsStringOption.xml new file mode 100644 index 0000000000..de8351cf22 --- /dev/null +++ b/src/gromacs/options/tests/refdata/TreeValueSupportTest_SupportsStringOption.xml @@ -0,0 +1,8 @@ + + + + + + s + + diff --git a/src/gromacs/options/tests/treesupport.cpp b/src/gromacs/options/tests/treesupport.cpp index cf14707333..390a375f23 100644 --- a/src/gromacs/options/tests/treesupport.cpp +++ b/src/gromacs/options/tests/treesupport.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2016, by the GROMACS development team, led by + * Copyright (c) 2016,2017, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -218,4 +218,82 @@ TEST_F(TreeValueSupportAdjustTest, OrdersValues) runTest(); } +/******************************************************************** + * Support for different option types + */ + +class TreeValueSupportTest : public ::testing::Test +{ + public: + void runTest() + { + gmx::test::TestReferenceData refdata; + gmx::test::TestReferenceChecker checker(refdata.rootChecker()); + gmx::KeyValueTreeObject tree(builder_.build()); + checker.checkKeyValueTreeObject(tree, "Input"); + ASSERT_NO_THROW_GMX(tree = gmx::adjustKeyValueTreeFromOptions(tree, options_)); + checker.checkKeyValueTreeObject(tree, "Adjusted"); + } + + gmx::Options options_; + gmx::KeyValueTreeBuilder builder_; +}; + +TEST_F(TreeValueSupportTest, SupportsBooleanOption) +{ + options_.addOption(gmx::BooleanOption("a").defaultValue(true)); + runTest(); +} + +TEST_F(TreeValueSupportTest, SupportsIntegerOption) +{ + options_.addOption(gmx::IntegerOption("a").defaultValue(2)); + runTest(); +} + +TEST_F(TreeValueSupportTest, SupportsInt64Option) +{ + options_.addOption(gmx::Int64Option("a").defaultValue(2)); + runTest(); +} + +TEST_F(TreeValueSupportTest, SupportsStringOption) +{ + options_.addOption(gmx::StringOption("a").defaultValue("s")); + runTest(); +} + +TEST_F(TreeValueSupportTest, SupportsFloatOption) +{ + options_.addOption(gmx::FloatOption("a").defaultValue(1.5)); + runTest(); +} + +TEST_F(TreeValueSupportTest, SupportsDoubleOption) +{ + options_.addOption(gmx::DoubleOption("a").defaultValue(1.5)); + runTest(); +} + +TEST_F(TreeValueSupportTest, SupportsEnumIntOption) +{ + const char *const values[] = {"foo", "bar"}; + options_.addOption(gmx::EnumIntOption("a").enumValue(values).defaultValue(0)); + runTest(); +} + +//! Enum for testing EnumOption. +enum class TestEnum +{ + Foo, Bar +}; + +TEST_F(TreeValueSupportTest, SupportsEnumOption) +{ + const char *const values[] = {"foo", "bar"}; + options_.addOption(gmx::EnumOption("a").enumValue(values) + .defaultValue(TestEnum::Foo)); + runTest(); +} + } // namespace diff --git a/src/gromacs/utility/tests/keyvaluetreeserializer.cpp b/src/gromacs/utility/tests/keyvaluetreeserializer.cpp index f94866c8a6..6f2e092c07 100644 --- a/src/gromacs/utility/tests/keyvaluetreeserializer.cpp +++ b/src/gromacs/utility/tests/keyvaluetreeserializer.cpp @@ -38,6 +38,7 @@ #include +#include "gromacs/utility/inmemoryserializer.h" #include "gromacs/utility/iserializer.h" #include "gromacs/utility/keyvaluetreebuilder.h" @@ -46,11 +47,11 @@ namespace { -class RefDataWriteSerializer : public gmx::ISerializer +class RefDataSerializer : public gmx::ISerializer { public: - RefDataWriteSerializer(gmx::test::TestReferenceChecker *parentChecker, - const char *id) + RefDataSerializer(gmx::test::TestReferenceChecker *parentChecker, + const char *id) : checker_(parentChecker->checkCompound("SerializedData", id)) { } @@ -86,46 +87,6 @@ class RefDataWriteSerializer : public gmx::ISerializer gmx::test::TestReferenceChecker checker_; }; -class RefDataReadSerializer : public gmx::ISerializer -{ - public: - RefDataReadSerializer(gmx::test::TestReferenceChecker *parentChecker, - const char *id) - : checker_(parentChecker->checkCompound("SerializedData", id)) - { - } - - virtual bool reading() const { return true; } - - virtual void doUChar(unsigned char *value) - { - *value = checker_.readUChar(nullptr); - } - virtual void doInt(int *value) - { - *value = checker_.readInteger(nullptr); - } - virtual void doInt64(gmx_int64_t *value) - { - *value = checker_.readInt64(nullptr); - } - virtual void doFloat(float *value) - { - *value = checker_.readFloat(nullptr); - } - virtual void doDouble(double *value) - { - *value = checker_.readDouble(nullptr); - } - virtual void doString(std::string *value) - { - *value = checker_.readString(nullptr); - } - - private: - gmx::test::TestReferenceChecker checker_; -}; - class KeyValueTreeSerializerTest : public ::testing::Test { public: @@ -136,18 +97,27 @@ class KeyValueTreeSerializerTest : public ::testing::Test gmx::test::TestReferenceChecker checker(data.rootChecker()); checker.checkKeyValueTreeObject(input, "Input"); { - RefDataWriteSerializer serializer(&checker, "Stream"); + RefDataSerializer serializer(&checker, "Stream"); gmx::serializeKeyValueTree(input, &serializer); } + std::vector buffer = serializeTree(input); { - RefDataReadSerializer serializer(&checker, "Stream"); + gmx::InMemoryDeserializer deserializer(buffer); gmx::KeyValueTreeObject output - = gmx::deserializeKeyValueTree(&serializer); + = gmx::deserializeKeyValueTree(&deserializer); checker.checkKeyValueTreeObject(output, "Input"); } } gmx::KeyValueTreeBuilder builder_; + + private: + std::vector serializeTree(const gmx::KeyValueTreeObject &tree) + { + gmx::InMemorySerializer serializer; + gmx::serializeKeyValueTree(tree, &serializer); + return serializer.finishAndGetBuffer(); + } }; TEST_F(KeyValueTreeSerializerTest, EmptyTree) diff --git a/src/testutils/refdata.cpp b/src/testutils/refdata.cpp index 474feff22f..2d80f779f7 100644 --- a/src/testutils/refdata.cpp +++ b/src/testutils/refdata.cpp @@ -982,6 +982,10 @@ void TestReferenceChecker::checkVariant(const Variant &variant, const char *id) { checkInteger(variant.cast(), id); } + else if (variant.isType()) + { + checkInt64(variant.cast(), id); + } else if (variant.isType()) { checkFloat(variant.cast(), id);