Refactor TestReferenceData lifetime management

Keep a global instance of the TestReferenceData implementation object,
and add a test event listener that interacts with it.  This allows
moving the final processing for the reference data out of the
destructor, making it behave better with exceptions, and still keeping
the same simple interface that the class provides.  This enables further
refactoring that would otherwise add a lot more logic into the
destructor.  Also, better sanity checks are possible for incorrect usage
of the reference data (e.g., creating multiple instances within the same
test).

Move libxml2 cleanup into the same listener to have it done at a more
appropriate step.

Change-Id: I69f85aca68a0e0374093ea169bd4ef6c0431caff

diff --git a/src/testutils/refdata.cpp b/src/testutils/refdata.cpp
index 263ad7d53fb87311fb29b32d640ed13a87cd3212..afed65abbaced4e911abceaaae6aa85452aaed8d 100644 (file)
--- a/src/testutils/refdata.cpp
+++ b/src/testutils/refdata.cpp
@@ -64,61 +64,24 @@
 #include "testutils/testexceptions.h"
 #include "testutils/testfilemanager.h"
 
-namespace
-{
-
-/*! \internal \brief
- * Global test environment for freeing up libxml2 internal buffers.
- */
-class TestReferenceDataEnvironment : public ::testing::Environment
-{
-    public:
-        //! Frees internal buffers allocated by libxml2.
-        virtual void TearDown()
-        {
-            xmlCleanupParser();
-        }
-};
-
-//! Global reference data mode set with gmx::test::setReferenceDataMode().
-// TODO: Make this a real enum; requires solving a TODO in StringOption.
-int g_referenceDataMode = gmx::test::erefdataCompare;
-
-//! Returns the global reference data mode.
-gmx::test::ReferenceDataMode getReferenceDataMode()
-{
-    return static_cast<gmx::test::ReferenceDataMode>(g_referenceDataMode);
-}
-
-} // namespace
-
 namespace gmx
 {
 namespace test
 {
 
-void initReferenceData(IOptionsContainer *options)
-{
-    // Needs to correspond to the enum order in refdata.h.
-    const char *const refDataEnum[] = { "check", "create", "update" };
-    options->addOption(
-            StringOption("ref-data").enumValue(refDataEnum)
-                .defaultEnumIndex(0)
-                .storeEnumIndex(&g_referenceDataMode)
-                .description("Operation mode for tests that use reference data"));
-    ::testing::AddGlobalTestEnvironment(new TestReferenceDataEnvironment);
-}
-
 /********************************************************************
- * TestReferenceData::Impl
+ * TestReferenceData::Impl declaration
  */
 
+namespace internal
+{
+
 /*! \internal \brief
  * Private implementation class for TestReferenceData.
  *
  * \ingroup module_testutils
  */
-class TestReferenceData::Impl
+class TestReferenceDataImpl
 {
     public:
         //! String constant for output XML version string.
@@ -131,8 +94,11 @@ class TestReferenceData::Impl
         static const xmlChar * const cRootNodeName;
 
         //! Initializes a checker in the given mode.
-        Impl(ReferenceDataMode mode, bool bSelfTestMode);
-        ~Impl();
+        TestReferenceDataImpl(ReferenceDataMode mode, bool bSelfTestMode);
+        ~TestReferenceDataImpl();
+
+        //! Performs final reference data processing when test ends.
+        void onTestEnd();
 
         //! Full path of the reference data file.
         std::string             fullFilename_;
@@ -155,17 +121,114 @@ class TestReferenceData::Impl
         bool                    bInUse_;
 };
 
-const xmlChar * const TestReferenceData::Impl::cXmlVersion =
+}       // namespace internal
+
+/********************************************************************
+ * Internal helpers
+ */
+
+namespace
+{
+
+//! Convenience typedef for a smart pointer to TestReferenceDataImpl;
+typedef boost::shared_ptr<internal::TestReferenceDataImpl>
+    TestReferenceDataImplPointer;
+
+/*! \brief
+ * Global reference data instance.
+ *
+ * The object is created when the test creates a TestReferenceData, and the
+ * object is destructed (and other post-processing is done) at the end of each
+ * test by ReferenceDataTestEventListener (which is installed as a Google Test
+ * test listener).
+ */
+TestReferenceDataImplPointer g_referenceData;
+//! Global reference data mode set with setReferenceDataMode().
+// TODO: Make this a real enum; requires solving a TODO in StringOption.
+int                          g_referenceDataMode = erefdataCompare;
+
+//! Returns the global reference data mode.
+ReferenceDataMode getReferenceDataMode()
+{
+    return static_cast<ReferenceDataMode>(g_referenceDataMode);
+}
+
+//! Returns a reference to the global reference data object.
+TestReferenceDataImplPointer initReferenceDataInstance()
+{
+    GMX_RELEASE_ASSERT(!g_referenceData,
+                       "Test cannot create multiple TestReferenceData instances");
+    g_referenceData.reset(new internal::TestReferenceDataImpl(getReferenceDataMode(), false));
+    return g_referenceData;
+}
+
+//! Handles reference data creation for self-tests.
+TestReferenceDataImplPointer initReferenceDataInstanceForSelfTest(ReferenceDataMode mode)
+{
+    if (g_referenceData)
+    {
+        g_referenceData->onTestEnd();
+        g_referenceData.reset();
+    }
+    g_referenceData.reset(new internal::TestReferenceDataImpl(mode, true));
+    return g_referenceData;
+}
+
+class ReferenceDataTestEventListener : public ::testing::EmptyTestEventListener
+{
+    public:
+        virtual void OnTestEnd(const ::testing::TestInfo &)
+        {
+            if (g_referenceData)
+            {
+                GMX_RELEASE_ASSERT(g_referenceData.unique(),
+                                   "Test leaked TestRefeferenceData objects");
+                g_referenceData->onTestEnd();
+                g_referenceData.reset();
+            }
+        }
+
+        // Frees internal buffers allocated by libxml2.
+        virtual void OnTestProgramEnd(const ::testing::UnitTest &)
+        {
+            xmlCleanupParser();
+        }
+};
+
+}       // namespace
+
+void initReferenceData(IOptionsContainer *options)
+{
+    // Needs to correspond to the enum order in refdata.h.
+    const char *const refDataEnum[] = { "check", "create", "update" };
+    options->addOption(
+            StringOption("ref-data").enumValue(refDataEnum)
+                .defaultEnumIndex(0)
+                .storeEnumIndex(&g_referenceDataMode)
+                .description("Operation mode for tests that use reference data"));
+    ::testing::UnitTest::GetInstance()->listeners().Append(
+            new ReferenceDataTestEventListener);
+}
+
+/********************************************************************
+ * TestReferenceDataImpl definition
+ */
+
+namespace internal
+{
+
+const xmlChar * const TestReferenceDataImpl::cXmlVersion =
     (const xmlChar *)"1.0";
-const xmlChar * const TestReferenceData::Impl::cXmlStyleSheetNodeName =
+const xmlChar * const TestReferenceDataImpl::cXmlStyleSheetNodeName =
     (const xmlChar *)"xml-stylesheet";
-const xmlChar * const TestReferenceData::Impl::cXmlStyleSheetContent =
+const xmlChar * const TestReferenceDataImpl::cXmlStyleSheetContent =
     (const xmlChar *)"type=\"text/xsl\" href=\"referencedata.xsl\"";
-const xmlChar * const TestReferenceData::Impl::cRootNodeName =
+const xmlChar * const TestReferenceDataImpl::cRootNodeName =
     (const xmlChar *)"ReferenceData";
 
 
-TestReferenceData::Impl::Impl(ReferenceDataMode mode, bool bSelfTestMode)
+TestReferenceDataImpl::TestReferenceDataImpl(
+        ReferenceDataMode mode, bool bSelfTestMode)
     : refDoc_(NULL), bWrite_(false), bSelfTestMode_(bSelfTestMode), bInUse_(false)
 {
     const std::string dirname =
@@ -221,8 +284,15 @@ TestReferenceData::Impl::Impl(ReferenceDataMode mode, bool bSelfTestMode)
     }
 }
 
+TestReferenceDataImpl::~TestReferenceDataImpl()
+{
+    if (refDoc_ != NULL)
+    {
+        xmlFreeDoc(refDoc_);
+    }
+}
 
-TestReferenceData::Impl::~Impl()
+void TestReferenceDataImpl::onTestEnd()
 {
     if (bWrite_ && bInUse_ && refDoc_ != NULL)
     {
@@ -239,12 +309,10 @@ TestReferenceData::Impl::~Impl()
             ADD_FAILURE() << "Saving reference data failed for " + fullFilename_;
         }
     }
-    if (refDoc_ != NULL)
-    {
-        xmlFreeDoc(refDoc_);
-    }
 }
 
+}       // namespace internal
+
 
 /********************************************************************
  * TestReferenceChecker::Impl
@@ -622,13 +690,13 @@ TestReferenceChecker::Impl::shouldIgnore() const
  */
 
 TestReferenceData::TestReferenceData()
-    : impl_(new Impl(getReferenceDataMode(), false))
+    : impl_(initReferenceDataInstance())
 {
 }
 
 
 TestReferenceData::TestReferenceData(ReferenceDataMode mode)
-    : impl_(new Impl(mode, true))
+    : impl_(initReferenceDataInstanceForSelfTest(mode))
 {
 }
 

diff --git a/src/testutils/refdata.h b/src/testutils/refdata.h
index 083fbf38a970226f1dcac0605de4d7f10a353827..0c4d7ca3a0d89cd89628ac79cde1cfb7a66140ba 100644 (file)
--- a/src/testutils/refdata.h
+++ b/src/testutils/refdata.h
@@ -46,6 +46,8 @@
 #include <iterator>
 #include <string>
 
+#include <boost/shared_ptr.hpp>
+
 #include "gromacs/utility/basedefinitions.h"
 #include "gromacs/utility/classhelpers.h"
 
@@ -107,14 +109,19 @@ void initReferenceData(IOptionsContainer *options);
 
 class TestReferenceChecker;
 
+namespace internal
+{
+class TestReferenceDataImpl;
+}
+
 /*! \libinternal \brief
  * Handles creation of and comparison to test reference data.
  *
  * This class provides functionality to use the same code to generate reference
  * data and then on later runs compare the results of the code against that
  * reference.  The mode in which the class operates (writing reference data or
- * comparing against existing data) is set with parseReferenceDataArgs(), which
- * is automatically called when using the testutils module to implement tests.
+ * comparing against existing data) is set using a command-line option that
+ * is automatically managed when using the testutils module to implement tests.
  * Tests only need to create an instance of TestReferenceData, obtain a
  * TestReferenceChecker using the rootChecker() method and use the various
  * check*() methods in TestReferenceChecker to indicate values to check.  If
@@ -176,8 +183,8 @@ class TestReferenceData
         /*! \brief
          * Frees reference data structures.
          *
-         * In the current implementation, this function writes the reference
-         * data out if necessary.
+         * The reference data is written out if necessary automatically when
+         * the test finishes.
          */
         ~TestReferenceData();
 
@@ -192,9 +199,9 @@ class TestReferenceData
         TestReferenceChecker rootChecker();
 
     private:
-        class Impl;
+        boost::shared_ptr<internal::TestReferenceDataImpl> impl_;
 
-        PrivateImplPointer<Impl> impl_;
+        GMX_DISALLOW_COPY_AND_ASSIGN(TestReferenceData);
 };
 
 /*! \libinternal \brief

diff --git a/src/testutils/testinit.cpp b/src/testutils/testinit.cpp
index a8bead662c4ba9a0118d619ebd7d9b1bc89d5c07..8fb4864017a804dfec2b4473d80f0bbd2ace9d03 100644 (file)
--- a/src/testutils/testinit.cpp
+++ b/src/testutils/testinit.cpp
@@ -190,6 +190,12 @@ void initTestUtils(const char *dataPath, const char *tempPath, int *argc, char *
         // TODO: Make this into a FileNameOption (or a DirectoryNameOption).
         options.addOption(StringOption("src-root").store(&sourceRoot)
                               .description("Override source tree location (for data files)"));
+        // The potential MPI test event listener must be initialized first,
+        // because it should appear in the start of the event listener list,
+        // before other event listeners that may generate test failures
+        // (currently, such an event listener is used by the reference data
+        // framework).
+        initMPIOutput();
         // TODO: Consider removing this option from test binaries that do not need it.
         initReferenceData(&options);
         initTestOptions(&options);
@@ -214,7 +220,6 @@ void initTestUtils(const char *dataPath, const char *tempPath, int *argc, char *
                     Path::join(sourceRoot, dataPath));
         }
         setFatalErrorHandler(NULL);
-        initMPIOutput();
     }
     catch (const std::exception &ex)
     {