Skip to content

Commit

Permalink
[7.x][ML] Improve program counter resets between unit tests (elastic#…
Browse files Browse the repository at this point in the history
…1658)

Program counters are implemented as a singleton object,
which means their values spill between unit tests unless
action is taken to prevent this.  Often it doesn't matter,
as most unit tests do not care about program counter
values.

Two unit test suites already reset program counters in
between tests: CProgramCountersTest and
CRestorePreviousStateTest.

It turns out that a third test suite should have been
resetting counters but wasn't:
CDataFrameAnalysisInstrumentationTest.

This PR rectifies that omission and also factors out the
code used to reset counters into the test library which
makes it easier to reuse if other suites also need the
functionality in the future.

Backport of elastic#1656
  • Loading branch information
droberts195 authored Jan 12, 2021
1 parent fa776dd commit c3c03aa
Show file tree
Hide file tree
Showing 8 changed files with 89 additions and 49 deletions.
6 changes: 4 additions & 2 deletions include/core/CProgramCounters.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include <vector>

namespace CProgramCountersTest {
class CTestFixture;
class CProgramCountersTestRunner;
struct testCounters;
struct testUnknownCounter;
Expand All @@ -28,6 +27,9 @@ struct testPersist;
}

namespace ml {
namespace test {
class CProgramCounterClearingFixture;
}
namespace counter_t {

//! The enum values must be explicitly assigned & names should have a meaningful prefix to effectively namespace counters
Expand Down Expand Up @@ -354,7 +356,7 @@ class CORE_EXPORT CProgramCounters {
const CProgramCounters& counters);

//! Befriend the test suite
friend class CProgramCountersTest::CTestFixture;
friend class test::CProgramCounterClearingFixture;
friend class CProgramCountersTest::CProgramCountersTestRunner;
friend struct CProgramCountersTest::testCounters;
friend struct CProgramCountersTest::testUnknownCounter;
Expand Down
37 changes: 37 additions & 0 deletions include/test/CProgramCounterClearingFixture.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
#ifndef INCLUDED_ml_test_CProgramCounterClearingFixture_h
#define INCLUDED_ml_test_CProgramCounterClearingFixture_h

#include <test/ImportExport.h>

namespace ml {
namespace test {

//! \brief
//! Test fixture that resets all program counters to zero
//! between tests.
//!
//! DESCRIPTION:\n
//! Program counters are implemented as a singleton object,
//! which means their values are remembered across unit tests.
//! Unit tests that care about the values of program counters
//! should use this test fixture to ensure they start from a
//! well known state. Otherwise test results can vary depending
//! on whether a test is run alone or as part of a suite.
//!
//! IMPLEMENTATION DECISIONS:\n
//! This functionality is required in multiple unit test suites,
//! hence it's in the test library.
//!
class TEST_EXPORT CProgramCounterClearingFixture {
public:
CProgramCounterClearingFixture();
};
}
}

#endif // INCLUDED_ml_test_CProgramCounterClearingFixture_h
10 changes: 5 additions & 5 deletions lib/api/unittest/CDataFrameAnalysisInstrumentationTest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
#include <boost/test/tools/interface.hpp>
#include <core/CTimeUtils.h>

#include <api/CDataFrameAnalysisInstrumentation.h>

#include <test/BoostTestCloseAbsolute.h>
#include <test/CDataFrameAnalysisSpecificationFactory.h>
#include <test/CDataFrameAnalyzerTrainingFactory.h>
#include <test/CProgramCounterClearingFixture.h>

#include <rapidjson/schema.h>

Expand Down Expand Up @@ -99,7 +99,7 @@ void addOutlierTestData(TStrVec fieldNames,
}
}

BOOST_AUTO_TEST_CASE(testMemoryState) {
BOOST_FIXTURE_TEST_CASE(testMemoryState, ml::test::CProgramCounterClearingFixture) {
std::string jobId{"testJob"};
std::size_t memoryLimit{core::constants::BYTES_IN_GIGABYTES};
std::int64_t memoryUsage{500000};
Expand Down Expand Up @@ -141,7 +141,7 @@ BOOST_AUTO_TEST_CASE(testMemoryState) {
BOOST_TEST_REQUIRE(hasMemoryUsage);
}

BOOST_AUTO_TEST_CASE(testTrainingRegression) {
BOOST_FIXTURE_TEST_CASE(testTrainingRegression, ml::test::CProgramCounterClearingFixture) {
std::stringstream output;
auto outputWriterFactory = [&output]() {
return std::make_unique<core::CJsonOutputStreamWrapper>(output);
Expand Down Expand Up @@ -234,7 +234,7 @@ BOOST_AUTO_TEST_CASE(testTrainingRegression) {
BOOST_TEST_REQUIRE(hasMemoryUsage);
}

BOOST_AUTO_TEST_CASE(testTrainingClassification) {
BOOST_FIXTURE_TEST_CASE(testTrainingClassification, ml::test::CProgramCounterClearingFixture) {
std::stringstream output;
auto outputWriterFactory = [&output]() {
return std::make_unique<core::CJsonOutputStreamWrapper>(output);
Expand Down Expand Up @@ -298,7 +298,7 @@ BOOST_AUTO_TEST_CASE(testTrainingClassification) {
BOOST_TEST_REQUIRE(initialMemoryReport);
}

BOOST_AUTO_TEST_CASE(testOutlierDetection) {
BOOST_FIXTURE_TEST_CASE(testOutlierDetection, ml::test::CProgramCounterClearingFixture) {
std::stringstream output;
auto outputWriterFactory = [&output]() {
return std::make_unique<core::CJsonOutputStreamWrapper>(output);
Expand Down
1 change: 0 additions & 1 deletion lib/api/unittest/CInferenceModelMetadataTest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
#include <boost/test/tools/interface.hpp>
#include <core/CJsonOutputStreamWrapper.h>

#include <maths/CBoostedTreeLoss.h>
Expand Down
29 changes: 9 additions & 20 deletions lib/api/unittest/CRestorePreviousStateTest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include <core/CJsonOutputStreamWrapper.h>
#include <core/CLogger.h>
#include <core/COsFileFuncs.h>
#include <core/CProgramCounters.h>

#include <model/CAnomalyDetectorModelConfig.h>
#include <model/CLimits.h>
Expand All @@ -19,6 +18,8 @@
#include <api/CSingleStreamSearcher.h>
#include <api/CStateRestoreStreamFilter.h>

#include <test/CProgramCounterClearingFixture.h>

#include "CTestAnomalyJob.h"
#include "CTestFieldDataCategorizer.h"

Expand All @@ -33,18 +34,6 @@

BOOST_AUTO_TEST_SUITE(CRestorePreviousStateTest)

class CTestFixture {
public:
CTestFixture() {
ml::core::CProgramCounters& counters = ml::core::CProgramCounters::instance();

// Set all counters to 0
for (std::size_t i = 0; i < ml::counter_t::NUM_COUNTERS; ++i) {
counters.counter(i) = 0;
}
}
};

namespace {

void reportPersistComplete(ml::api::CModelSnapshotJsonWriter::SModelSnapshotReport modelSnapshotReport,
Expand Down Expand Up @@ -228,7 +217,7 @@ void anomalyDetectorRestoreHelper(const std::string& stateFile,
}
}

BOOST_FIXTURE_TEST_CASE(testRestoreDetectorBy, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testRestoreDetectorBy, ml::test::CProgramCounterClearingFixture) {
for (const auto& version : BWC_VERSIONS) {
LOG_INFO(<< "Test restoring state from version " << version.s_Version);
anomalyDetectorRestoreHelper(
Expand All @@ -237,7 +226,7 @@ BOOST_FIXTURE_TEST_CASE(testRestoreDetectorBy, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testRestoreDetectorOver, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testRestoreDetectorOver, ml::test::CProgramCounterClearingFixture) {
for (const auto& version : BWC_VERSIONS) {
LOG_INFO(<< "Test restoring state from version " << version.s_Version);
anomalyDetectorRestoreHelper("testfiles/state/" + version.s_Version + "/over_detector_state.json",
Expand All @@ -246,7 +235,7 @@ BOOST_FIXTURE_TEST_CASE(testRestoreDetectorOver, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testRestoreDetectorPartition, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testRestoreDetectorPartition, ml::test::CProgramCounterClearingFixture) {
for (const auto& version : BWC_VERSIONS) {
LOG_INFO(<< "Test restoring state from version " << version.s_Version);
anomalyDetectorRestoreHelper("testfiles/state/" + version.s_Version + "/partition_detector_state.json",
Expand All @@ -255,7 +244,7 @@ BOOST_FIXTURE_TEST_CASE(testRestoreDetectorPartition, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testRestoreDetectorDc, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testRestoreDetectorDc, ml::test::CProgramCounterClearingFixture) {
for (const auto& version : BWC_VERSIONS) {
LOG_INFO(<< "Test restoring state from version " << version.s_Version);
anomalyDetectorRestoreHelper(
Expand All @@ -264,7 +253,7 @@ BOOST_FIXTURE_TEST_CASE(testRestoreDetectorDc, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testRestoreDetectorCount, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testRestoreDetectorCount, ml::test::CProgramCounterClearingFixture) {
for (const auto& version : BWC_VERSIONS) {
LOG_INFO(<< "Test restoring state from version " << version.s_Version);
anomalyDetectorRestoreHelper("testfiles/state/" + version.s_Version + "/count_detector_state.json",
Expand All @@ -273,7 +262,7 @@ BOOST_FIXTURE_TEST_CASE(testRestoreDetectorCount, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testRestoreNormalizer, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testRestoreNormalizer, ml::test::CProgramCounterClearingFixture) {
for (const auto& version : BWC_VERSIONS) {
ml::model::CAnomalyDetectorModelConfig modelConfig =
ml::model::CAnomalyDetectorModelConfig::defaultConfig(3600);
Expand All @@ -284,7 +273,7 @@ BOOST_FIXTURE_TEST_CASE(testRestoreNormalizer, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testRestoreCategorizer, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testRestoreCategorizer, ml::test::CProgramCounterClearingFixture) {
for (const auto& version : BWC_VERSIONS) {
LOG_INFO(<< "Test restoring state from version " << version.s_Version);
categorizerRestoreHelper("testfiles/state/" + version.s_Version + "/categorizer_state.json",
Expand Down
28 changes: 7 additions & 21 deletions lib/core/unittest/CProgramCountersTest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <core/CRegex.h>
#include <core/CThread.h>

#include <test/CProgramCounterClearingFixture.h>
#include <test/CRandomNumbers.h>

#include <boost/test/unit_test.hpp>
Expand All @@ -24,21 +25,6 @@ BOOST_AUTO_TEST_SUITE(CProgramCountersTest)

const int TEST_COUNTER{0u};

class CTestFixture {
public:
CTestFixture() {
ml::core::CProgramCounters& counters = ml::core::CProgramCounters::instance();

// Set all counters to 0
for (std::size_t i = 0; i < ml::counter_t::NUM_COUNTERS; ++i) {
counters.counter(i) = 0;
}

// Clear the cache
counters.m_Cache.clear();
}
};

class CProgramCountersTestRunner : public ml::core::CThread {
public:
static const int N = 6;
Expand Down Expand Up @@ -89,7 +75,7 @@ void restore(const std::string& staticsXml) {
&ml::core::CProgramCounters::staticsAcceptRestoreTraverser));
}

BOOST_FIXTURE_TEST_CASE(testCounters, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testCounters, ml::test::CProgramCounterClearingFixture) {
ml::core::CProgramCounters& counters = ml::core::CProgramCounters::instance();

using TCounter = ml::core::CProgramCounters::TCounter;
Expand Down Expand Up @@ -130,7 +116,7 @@ BOOST_FIXTURE_TEST_CASE(testCounters, CTestFixture) {
BOOST_REQUIRE_EQUAL(TCounter(0x1000000), counters.counter(TEST_COUNTER));
}

BOOST_FIXTURE_TEST_CASE(testPersist, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testPersist, ml::test::CProgramCounterClearingFixture) {
// Run the first set of checks without registering a specific subset of counters
// in order to test the entire set now and in the future
using TCounter = ml::core::CProgramCounters::TCounter;
Expand Down Expand Up @@ -278,7 +264,7 @@ BOOST_FIXTURE_TEST_CASE(testPersist, CTestFixture) {
BOOST_REQUIRE_EQUAL(outputOrder1, outputOrder2);
}

BOOST_FIXTURE_TEST_CASE(testUnknownCounter, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testUnknownCounter, ml::test::CProgramCounterClearingFixture) {
ml::core::CProgramCounters& counters = ml::core::CProgramCounters::instance();

// Name of the log file to use. It must match the name specified
Expand Down Expand Up @@ -309,7 +295,7 @@ BOOST_FIXTURE_TEST_CASE(testUnknownCounter, CTestFixture) {
std::remove(logFile);
}

BOOST_FIXTURE_TEST_CASE(testMissingCounter, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testMissingCounter, ml::test::CProgramCounterClearingFixture) {
// explicitly register interest in a particular set of counters
const ml::counter_t::TCounterTypeSet counterSet{
ml::counter_t::E_TSADNumberNewPeopleNotAllowed,
Expand Down Expand Up @@ -348,7 +334,7 @@ BOOST_FIXTURE_TEST_CASE(testMissingCounter, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testCacheCounters, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testCacheCounters, ml::test::CProgramCounterClearingFixture) {
ml::core::CProgramCounters& counters = ml::core::CProgramCounters::instance();

// confirm that initially the cache is empty
Expand Down Expand Up @@ -386,7 +372,7 @@ BOOST_FIXTURE_TEST_CASE(testCacheCounters, CTestFixture) {
}
}

BOOST_FIXTURE_TEST_CASE(testMax, CTestFixture) {
BOOST_FIXTURE_TEST_CASE(testMax, ml::test::CProgramCounterClearingFixture) {
ml::test::CRandomNumbers rng;
std::size_t m1{0}, m2{0};
std::thread thread1{[&m1, &rng] {
Expand Down
26 changes: 26 additions & 0 deletions lib/test/CProgramCounterClearingFixture.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

#include <test/CProgramCounterClearingFixture.h>

#include <core/CProgramCounters.h>

namespace ml {
namespace test {

CProgramCounterClearingFixture::CProgramCounterClearingFixture() {
core::CProgramCounters& counters{core::CProgramCounters::instance()};

// Set all counters to 0
for (std::size_t i = 0; i < counter_t::NUM_COUNTERS; ++i) {
counters.counter(i) = 0;
}

// Clear the cache
counters.m_Cache.clear();
}
}
}
1 change: 1 addition & 0 deletions lib/test/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ SRCS= \
CDataFrameAnalyzerTrainingFactory.cc \
CMultiFileDataAdder.cc \
CMultiFileSearcher.cc \
CProgramCounterClearingFixture.cc \
CRandomNumbers.cc \
CTestObserver.cc \
CTestTmpDir.cc \
Expand Down

0 comments on commit c3c03aa

Please sign in to comment.