Check for unused refdata values
authorTeemu Murtola <teemu.murtola@gmail.com>
Mon, 1 Aug 2016 05:16:12 +0000 (08:16 +0300)
committerMark Abraham <mark.j.abraham@gmail.com>
Thu, 18 Aug 2016 15:54:17 +0000 (17:54 +0200)
Make a test using refdata fail if it does not produce some reference
values at all.  Use this instead of checkPresent() for some cases where
the new functionality is a clearer fit for the purpose.  It is possible
to trigger the check explicitly for a subtree, but if not done
otherwise, it is always done for all values at the end of the test.

Change-Id: I722abd978f0c8fa9dc279264ff90cdad02e02b9d

src/testutils/interactivetest.cpp
src/testutils/refdata-impl.h
src/testutils/refdata.cpp
src/testutils/refdata.h
src/testutils/tests/refdata_tests.cpp
src/testutils/tests/xvgtest_tests.cpp
src/testutils/xvgtest.cpp

index ce8c8541b52d659ddd71e88ca789ff3ed78a9f3a..da76c52d454f96125bff7d5bf5c9edfd6338abe1 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2015, by the GROMACS development team, led by
+ * Copyright (c) 2015,2016, 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.
@@ -122,19 +122,14 @@ class InteractiveTestHelper::Impl
 
         void checkOutput()
         {
-            const std::string id = formatString("Output%d", static_cast<int>(currentLine_));
-            if (checker_.checkPresent(bHasOutput_, id.c_str()))
+            if (bHasOutput_)
             {
+                const std::string id = formatString("Output%d", static_cast<int>(currentLine_));
                 StringTestBase::checkText(&checker_, currentOutput_, id.c_str());
+                bHasOutput_ = false;
             }
-            bHasOutput_ = false;
             currentOutput_.clear();
         }
-        void checkPendingInput()
-        {
-            const std::string id = formatString("Input%d", static_cast<int>(currentLine_+1));
-            checker_.checkPresent(false, id.c_str());
-        }
 
         TestReferenceChecker             checker_;
         ConstArrayRef<const char *>      inputLines_;
@@ -180,7 +175,7 @@ TextOutputStream &InteractiveTestHelper::outputStream()
 void InteractiveTestHelper::checkSession()
 {
     impl_->checkOutput();
-    impl_->checkPendingInput();
+    impl_->checker_.checkUnusedEntries();
 }
 
 } // namespace test
index 4d80baeee37727bd76db95255b0011e7140127ca..3ac5b644c9b20d070a038440560d8527386a24f7 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2015, by the GROMACS development team, led by
+ * Copyright (c) 2015,2016, 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.
@@ -67,7 +67,7 @@ class ReferenceDataEntry
 
         ReferenceDataEntry(const char *type, const char *id)
             : type_(type), id_(id != NULL ? id : ""), isTextBlock_(false),
-              correspondingOutputEntry_(NULL)
+              hasBeenChecked_(false), correspondingOutputEntry_(NULL)
         {
         }
 
@@ -132,6 +132,18 @@ class ReferenceDataEntry
             return prev != children_.end();
         }
 
+        bool hasBeenChecked() const { return hasBeenChecked_; }
+        void setChecked() { hasBeenChecked_ = true; }
+
+        void setCheckedIncludingChildren()
+        {
+            setChecked();
+            for (const auto &child : children_)
+            {
+                child->setCheckedIncludingChildren();
+            }
+        }
+
         EntryPointer cloneToOutputEntry()
         {
             EntryPointer entry(new ReferenceDataEntry(type_.c_str(), id_.c_str()));
@@ -179,6 +191,7 @@ class ReferenceDataEntry
         std::string         value_;
         bool                isTextBlock_;
         ChildList           children_;
+        bool                hasBeenChecked_;
         ReferenceDataEntry *correspondingOutputEntry_;
 };
 
index 2afd1ec41ce81d46c591da5175b16f8d23ccbefe..beb3369ae28a8596d8f4d56e33d886032134ea0c 100644 (file)
@@ -203,6 +203,67 @@ class ReferenceDataTestEventListener : public ::testing::EmptyTestEventListener
         }
 };
 
+//! Formats a path to a reference data entry with a non-null id.
+std::string formatEntryPath(const std::string &prefix, const std::string &id)
+{
+    return prefix + "/" + id;
+}
+
+//! Formats a path to a reference data entry with a null id.
+std::string formatSequenceEntryPath(const std::string &prefix, int seqIndex)
+{
+    return formatString("%s/[%d]", prefix.c_str(), seqIndex+1);
+}
+
+//! Finds all entries that have not been checked under a given root.
+void gatherUnusedEntries(const ReferenceDataEntry &root,
+                         const std::string        &rootPath,
+                         std::vector<std::string> *unusedPaths)
+{
+    if (!root.hasBeenChecked())
+    {
+        unusedPaths->push_back(rootPath);
+        return;
+    }
+    int seqIndex = 0;
+    for (const auto &child : root.children())
+    {
+        std::string path;
+        if (child->id().empty())
+        {
+            path = formatSequenceEntryPath(rootPath, seqIndex);
+            ++seqIndex;
+        }
+        else
+        {
+            path = formatEntryPath(rootPath, child->id());
+        }
+        gatherUnusedEntries(*child, path, unusedPaths);
+    }
+}
+
+//! Produces a GTest assertion of any entries under given root have not been checked.
+void checkUnusedEntries(const ReferenceDataEntry &root, const std::string &rootPath)
+{
+    std::vector<std::string> unusedPaths;
+    gatherUnusedEntries(root, rootPath, &unusedPaths);
+    if (!unusedPaths.empty())
+    {
+        std::string paths;
+        if (unusedPaths.size() > 5)
+        {
+            paths = joinStrings(unusedPaths.begin(), unusedPaths.begin() + 5, "\n  ");
+            paths = "  " + paths + "\n  ...";
+        }
+        else
+        {
+            paths = joinStrings(unusedPaths.begin(), unusedPaths.end(), "\n  ");
+            paths = "  " + paths;
+        }
+        ADD_FAILURE() << "Reference data items not used in test:" << std::endl << paths;
+    }
+}
+
 }       // namespace
 
 void initReferenceData(IOptionsContainer *options)
@@ -276,18 +337,29 @@ TestReferenceDataImpl::TestReferenceDataImpl(
 
 void TestReferenceDataImpl::onTestEnd(bool testPassed)
 {
+    if (!bInUse_)
+    {
+        return;
+    }
     // TODO: Only write the file with update-changed if there were actual changes.
-    if (testPassed && bInUse_ && outputRootEntry_)
+    if (outputRootEntry_)
     {
-        std::string dirname = Path::getParentPath(fullFilename_);
-        if (!Directory::exists(dirname))
+        if (testPassed)
         {
-            if (Directory::create(dirname) != 0)
+            std::string dirname = Path::getParentPath(fullFilename_);
+            if (!Directory::exists(dirname))
             {
-                GMX_THROW(TestException("Creation of reference data directory failed: " + dirname));
+                if (Directory::create(dirname) != 0)
+                {
+                    GMX_THROW(TestException("Creation of reference data directory failed: " + dirname));
+                }
             }
+            writeReferenceDataFile(fullFilename_, *outputRootEntry_);
         }
-        writeReferenceDataFile(fullFilename_, *outputRootEntry_);
+    }
+    else if (compareRootEntry_)
+    {
+        checkUnusedEntries(*compareRootEntry_, "");
     }
 }
 
@@ -468,7 +540,7 @@ class TestReferenceChecker::Impl
         /*! \brief
          * Current number of unnamed elements in a sequence.
          *
-         * It is the index of the next added unnamed element.
+         * It is the index of the current unnamed element.
          */
         int                     seqIndex_;
 };
@@ -488,7 +560,7 @@ const char *const TestReferenceChecker::Impl::cSequenceLengthName = "Length";
 TestReferenceChecker::Impl::Impl(bool initialized)
     : initialized_(initialized), defaultTolerance_(defaultRealTolerance()),
       compareRootEntry_(NULL), outputRootEntry_(NULL),
-      updateMismatchingEntries_(false), bSelfTestMode_(false), seqIndex_(0)
+      updateMismatchingEntries_(false), bSelfTestMode_(false), seqIndex_(-1)
 {
 }
 
@@ -498,11 +570,11 @@ TestReferenceChecker::Impl::Impl(const std::string &path,
                                  ReferenceDataEntry *outputRootEntry,
                                  bool updateMismatchingEntries, bool bSelfTestMode,
                                  const FloatingPointTolerance &defaultTolerance)
-    : initialized_(true), defaultTolerance_(defaultTolerance), path_(path + "/"),
+    : initialized_(true), defaultTolerance_(defaultTolerance), path_(path),
       compareRootEntry_(compareRootEntry), outputRootEntry_(outputRootEntry),
       lastFoundEntry_(compareRootEntry->children().end()),
       updateMismatchingEntries_(updateMismatchingEntries),
-      bSelfTestMode_(bSelfTestMode), seqIndex_(0)
+      bSelfTestMode_(bSelfTestMode), seqIndex_(-1)
 {
 }
 
@@ -510,15 +582,16 @@ TestReferenceChecker::Impl::Impl(const std::string &path,
 std::string
 TestReferenceChecker::Impl::appendPath(const char *id) const
 {
-    std::string printId = (id != NULL) ? id : formatString("[%d]", seqIndex_);
-    return path_ + printId;
+    return id != nullptr
+           ? formatEntryPath(path_, id)
+           : formatSequenceEntryPath(path_, seqIndex_);
 }
 
 
 ReferenceDataEntry *TestReferenceChecker::Impl::findEntry(const char *id)
 {
     ReferenceDataEntry::ChildIterator entry = compareRootEntry_->findChild(id, lastFoundEntry_);
-    seqIndex_ = (id == NULL) ? seqIndex_+1 : 0;
+    seqIndex_ = (id == nullptr) ? seqIndex_+1 : -1;
     if (compareRootEntry_->isValidChild(entry))
     {
         lastFoundEntry_ = entry;
@@ -556,6 +629,7 @@ TestReferenceChecker::Impl::processItem(const char *type, const char *id,
         return ::testing::AssertionFailure()
                << "Reference data item " << fullId << " not found";
     }
+    entry->setChecked();
     ::testing::AssertionResult result(checkEntry(*entry, fullId, type, checker));
     if (outputRootEntry_ != NULL && entry->correspondingOutputEntry() == NULL)
     {
@@ -616,6 +690,7 @@ TestReferenceChecker TestReferenceData::rootChecker()
     {
         return TestReferenceChecker(new TestReferenceChecker::Impl(true));
     }
+    impl_->compareRootEntry_->setChecked();
     return TestReferenceChecker(
             new TestReferenceChecker::Impl("", impl_->compareRootEntry_.get(),
                                            impl_->outputRootEntry_.get(),
@@ -672,6 +747,17 @@ void TestReferenceChecker::setDefaultTolerance(
 }
 
 
+void TestReferenceChecker::checkUnusedEntries()
+{
+    if (impl_->compareRootEntry_)
+    {
+        gmx::test::checkUnusedEntries(*impl_->compareRootEntry_, impl_->path_);
+        // Mark them checked so that they are reported only once.
+        impl_->compareRootEntry_->setCheckedIncludingChildren();
+    }
+}
+
+
 bool TestReferenceChecker::checkPresent(bool bPresent, const char *id)
 {
     if (impl_->shouldIgnore() || impl_->outputRootEntry_ != NULL)
@@ -712,6 +798,7 @@ TestReferenceChecker TestReferenceChecker::checkCompound(const char *type, const
         ADD_FAILURE() << "Reference data item " << fullId << " not found";
         return TestReferenceChecker(new Impl(true));
     }
+    entry->setChecked();
     if (impl_->updateMismatchingEntries_)
     {
         entry->makeCompound(type);
index 99763e12c3f59a08fa8a41e918bd37e8c9cc6484..9314d8bbe3892cad05b6ca2ddd508df242fad0ce 100644 (file)
@@ -274,6 +274,22 @@ class TestReferenceChecker
          */
         void setDefaultTolerance(const FloatingPointTolerance &tolerance);
 
+        /*! \brief
+         * Checks that all reference values have been compared against.
+         *
+         * All values under the compound represented by this checker are
+         * checked, and a non-fatal Google Test assertion is produced if some
+         * values have not been used.
+         *
+         * If not called explicitly, the same check will be done for all
+         * reference data values when the test ends.
+         *
+         * This method also marks the values used, so that subsequent checks
+         * (including the check at the end of the test) will not produce
+         * another assertion about the same values.
+         */
+        void checkUnusedEntries();
+
         /*! \brief
          * Checks whether a data item is present.
          *
index 0a13555be2cd1b5944aca4d8d47ea109cce0bf90..9f00a19dedca9f3d28e3c3ee89ade5c24737e7c2 100644 (file)
@@ -266,6 +266,75 @@ TEST(ReferenceDataTest, HandlesMissingData)
         TestReferenceChecker checker(data.rootChecker());
         EXPECT_NONFATAL_FAILURE(checker.checkInteger(1, "missing"), "");
         EXPECT_NONFATAL_FAILURE(checker.checkSequenceArray(5, seq, "missing"), "");
+        // Needed to not make the test fail because of unused "int" and "seq".
+        EXPECT_NONFATAL_FAILURE(checker.checkUnusedEntries(), "");
+    }
+}
+
+
+TEST(ReferenceDataTest, HandlesUncheckedData)
+{
+    const int seq[5] = { -1, 3, 5, 2, 4 };
+
+    {
+        TestReferenceData    data(gmx::test::erefdataUpdateAll);
+        TestReferenceChecker checker(data.rootChecker());
+        checker.checkInteger(1, "int");
+        checker.checkSequenceArray(5, seq, "seq");
+        checker.checkUnusedEntries();
+    }
+    {
+        TestReferenceData    data(gmx::test::erefdataCompare);
+        TestReferenceChecker checker(data.rootChecker());
+        checker.checkInteger(1, "int");
+        EXPECT_NONFATAL_FAILURE(checker.checkUnusedEntries(), "");
+    }
+}
+
+
+TEST(ReferenceDataTest, HandlesUncheckedDataInSequence)
+{
+    const int seq[5] = { -1, 3, 5, 2, 4 };
+
+    {
+        TestReferenceData    data(gmx::test::erefdataUpdateAll);
+        TestReferenceChecker checker(data.rootChecker());
+        checker.checkInteger(1, "int");
+        checker.checkSequenceArray(5, seq, "seq");
+        checker.checkUnusedEntries();
+    }
+    {
+        TestReferenceData    data(gmx::test::erefdataCompare);
+        TestReferenceChecker checker(data.rootChecker());
+        checker.checkInteger(1, "int");
+        EXPECT_NONFATAL_FAILURE(checker.checkSequenceArray(3, seq, "seq"), "");
+        // It might be nicer to not report the unused sequence entries also
+        // here, but both behaviors are quite OK.
+        EXPECT_NONFATAL_FAILURE(checker.checkUnusedEntries(), "");
+    }
+}
+
+
+TEST(ReferenceDataTest, HandlesUncheckedDataInCompound)
+{
+    {
+        TestReferenceData    data(gmx::test::erefdataUpdateAll);
+        TestReferenceChecker checker(data.rootChecker());
+        TestReferenceChecker compound(checker.checkCompound("Compound", "Compound"));
+        compound.checkInteger(1, "int1");
+        compound.checkInteger(2, "int2");
+        compound.checkUnusedEntries();
+        checker.checkInteger(1, "int");
+        checker.checkUnusedEntries();
+    }
+    {
+        TestReferenceData    data(gmx::test::erefdataCompare);
+        TestReferenceChecker checker(data.rootChecker());
+        TestReferenceChecker compound(checker.checkCompound("Compound", "Compound"));
+        compound.checkInteger(1, "int1");
+        EXPECT_NONFATAL_FAILURE(compound.checkUnusedEntries(), "");
+        checker.checkInteger(1, "int");
+        checker.checkUnusedEntries();
     }
 }
 
@@ -563,4 +632,24 @@ TEST(ReferenceDataTest, HandlesUpdateChangedWithCompoundChanges)
     }
 }
 
+TEST(ReferenceDataTest, HandlesUpdateChangedWithRemovedEntries)
+{
+    {
+        TestReferenceData    data(gmx::test::erefdataUpdateAll);
+        TestReferenceChecker checker(data.rootChecker());
+        checker.checkInteger(1, "int");
+        checker.checkString("Test", "string");
+    }
+    {
+        TestReferenceData    data(gmx::test::erefdataUpdateChanged);
+        TestReferenceChecker checker(data.rootChecker());
+        checker.checkInteger(2, "int");
+    }
+    {
+        TestReferenceData    data(gmx::test::erefdataCompare);
+        TestReferenceChecker checker(data.rootChecker());
+        checker.checkInteger(2, "int");
+    }
+}
+
 } // namespace
index 5621821b129458020fb8f405f5a766c342aa6d3e..37a336b89b678a058755699b5b947bac3b293baa 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2015, by the GROMACS development team, led by
+ * Copyright (c) 2015,2016, 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.
@@ -110,7 +110,7 @@ TEST(XvgTests, CheckMissing)
         gmx::test::TestReferenceData    data(gmx::test::erefdataCompare);
         gmx::test::TestReferenceChecker checker(data.rootChecker());
         gmx::StringInputStream          sis(input);
-        EXPECT_NONFATAL_FAILURE(checkXvgFile(&sis, &checker, XvgMatchSettings()), "absent");
+        EXPECT_NONFATAL_FAILURE(checkXvgFile(&sis, &checker, XvgMatchSettings()), "not used in test");
     }
 }
 
index b69e0c87bb24088bb0317b1b1b9c4769dc15eb09..560232fd3886ad798adda0d251da0cb5dd82d11d 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2015, by the GROMACS development team, led by
+ * Copyright (c) 2015,2016, 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.
@@ -140,10 +140,7 @@ void checkXvgFile(TextInputStream        *input,
                                   &checkXvgDataPoint);
         ++dataRowCount;
     }
-    if (settings.testData)
-    {
-        dataChecker.checkPresent(false, formatString("Row%d", dataRowCount).c_str());
-    }
+    dataChecker.checkUnusedEntries();
     legendChecker.checkTextBlock(legendText, "XvgLegend");
 }