CODEWITHSUNDEEP
c++googletest
T'n'E
T'n'E

Reputation: 616

Loading test sequence file in parameterised Google Test

I'm trying to load a test sequence in Google Test. I have several test sequences, so I'm trying to make a parameterised test that takes the directory to the test sequence (multiple files), but I'm getting segmentation faults in the destructor when I try to free the resources.

// Test sequence class
class TestSequence {
 public:
    TestSequence(const std::string& dir)
        : TestSequence(dir + "/Values1.csv", dir + "/Values2.csv",
                       dir + "/Values3.csv") {}

    TestSequence(const std::string& val1_file, const std::string& val2_file,
                 const std::string& val3_file)
        : val1_file_path(val1_file),
          val2_file_path(val2_file),
          val3_file_path(val3_file) {
        mp_val1_file = new std::ifstream(m_val1_file_path);
        mp_val2_file = new std::ifstream(m_val2_file_path);
        mp_val3_file = new std::ifstream(m_val3_file_path);
    }

    virtual ~TestSequence() {
        delete mp_flows_file;  // <- Segmentation fault
        delete mp_pres_file;
        delete mp_params_file;
    }

    bool NextValue(MyValueType * p_value) {
        // Do some parsing on the file
        ...
    }
 private:
    std::string val1_file_path;
    std::string val2_file_path;
    std::string val3_file_path;

    std::ifstream *mp_val1_file;
    std::ifstream *mp_val1_file;
    std::ifstream *mp_val1_file;
}

// Test case class
class AlgorithmTests
    : public testing::TestWithParam<TestSequence> {
 protected:
    // Unit under test, mocks, etc...

 public:
    VentilationDetectionAlgorithmTests(void) {
        // Setting up unit under tests...
    }
};


// Instantiate parameterised tests with paths to directories
INSTANTIATE_TEST_CASE_P(
    SomeSequences, AlgorithmTests,
    ::testing::Values(TestSequence("test/support/sequence1"),
                      TestSequence("test/support/sequence2")));

I have two tests written. I've added a breakpoint to the constructor and destructor of the test sequence and at the first line of each test. This results in the following:

  1. Sequence constructor is called once for each directory (expected)
  2. Sequence destructor is called once for each directory, in reverse order (unexpected)
  3. Sequence destructor is called again on the last directory (segmentation fault on delete)

Test is never reached.

I assume there is something I've misunderstood about either how Google Test is using the created parameters, or how I'm supposed to deal with the resources in C++.

Grateful for any input on this!

The stack trace:

test.out!TestSequence::~TestSequence()
(/path/to/project/test/test_Algorithm.cpp:60)
test.out!TestSequence::~TestSequence() (/path/to/project/test/test_Algorithm.cpp:58)
test.out!testing::internal::ValueArray2<TestSequence, TestSequence>::operator testing::internal::ParamGenerator<TestSequence><TestSequence>() const (/path/to/project/vendor/googletest/include/gtest/internal/gtest-param-util-generated.h:103)
test.out!gtest_LongSequencesAlgorithmTests_EvalGenerator_() (/path/to/project/test/test_Algorithm.cpp:170)
test.out!testing::internal::ParameterizedTestCaseInfo<AlgorithmTests>::RegisterTests() (/path/to/project/vendor/googletest/include/gtest/internal/gtest-param-util.h:554)
test.out!testing::internal::ParameterizedTestCaseRegistry::RegisterTests() (/path/to/project/vendor/googletest/include/gtest/internal/gtest-param-util.h:714)
test.out!testing::internal::UnitTestImpl::RegisterParameterizedTests() (/path/to/project/vendor/googletest/src/gtest.cc:2620)
test.out!testing::internal::UnitTestImpl::PostFlagParsingInit() (/path/to/project/vendor/googletest/src/gtest.cc:4454)
test.out!void testing::internal::InitGoogleTestImpl<char>(int*, char**) (/path/to/project/vendor/googletest/src/gtest.cc:5356)
test.out!testing::InitGoogleTest(int*, char**) (/path/to/project/vendor/googletest/src/gtest.cc:5374)
test.out!void testing::internal::InitGoogleMockImpl<char>(int*, char**) (/path/to/project/vendor/googlemock/src/gmock.cc:131)
test.out!testing::InitGoogleMock(int*, char**) (/path/to/project/vendor/googlemock/src/gmock.cc:174)
test.out!main (/path/to/project/test/test_Main.cpp:13)
libdyld.dylib!start (Unknown Source:0)
libdyld.dylib!start (Unknown Source:0)

Upvotes: 1

Views: 800

Answers (2)

Ted Lyngmo
Ted Lyngmo

Reputation: 117830

Here's a version similar to your original but without using pointers/new. When copied, the files are also opened by the new object and the positions and states are set as in the original object.

#include <string>
#include <fstream>
#include <stdexcept>

class TestSequence {
public:
    TestSequence(const std::string& dir)
        : TestSequence(dir + "/Values1.csv", dir + "/Values2.csv",
                       dir + "/Values3.csv")
    {}
    TestSequence(const std::string& val1_file, const std::string& val2_file,
                 const std::string& val3_file)
        : val1_file_path(val1_file),
          val2_file_path(val2_file),
          val3_file_path(val3_file),
          mp_val1_file(val1_file_path),
          mp_val2_file(val2_file_path),
          mp_val3_file(val3_file_path)
    {
        if(!(mp_val1_file && mp_val2_file && mp_val3_file))
            throw std::runtime_error("angst");
    }
    TestSequence(const TestSequence& rhs) :
        TestSequence(rhs.val1_file_path, rhs.val2_file_path, rhs.val3_file_path)
    {
        mp_val1_file.seekg(rhs.mp_val1_file.rdbuf()->pubseekoff(0, std::ios_base::cur, std::ios_base::in));
        mp_val2_file.seekg(rhs.mp_val2_file.rdbuf()->pubseekoff(0, std::ios_base::cur, std::ios_base::in));
        mp_val3_file.seekg(rhs.mp_val3_file.rdbuf()->pubseekoff(0, std::ios_base::cur, std::ios_base::in));
        mp_val1_file.setstate(rhs.mp_val1_file.rdstate());
        mp_val2_file.setstate(rhs.mp_val2_file.rdstate());
        mp_val3_file.setstate(rhs.mp_val3_file.rdstate());
    }
    TestSequence(TestSequence&&) = default;
    TestSequence& operator=(const TestSequence& rhs) {
        TestSequence tmp(rhs);
        std::swap(*this, tmp);
        return *this;
    }
    TestSequence& operator=(TestSequence&&) = default;

    virtual ~TestSequence() {}

private:
    std::string val1_file_path;
    std::string val2_file_path;
    std::string val3_file_path;

    std::ifstream mp_val1_file;
    std::ifstream mp_val2_file;
    std::ifstream mp_val3_file;
};

Upvotes: 0

Gelldur
Gelldur

Reputation: 11558

You get SIGSEGV because you "copy" value of pointer (eg. 0x123123 address) and you make double free. So even if you set to nullptr it doesn't help because other copy of your TestSequence remebers old address.

If you want to avoid this you should override implicit version of copy ctor and assign copy operator of your class. Best solution is to use std::unique_ptr

Some information about implicit stuff.

Example:

#include <iostream>
#include <memory>
#include <string>
#include <fstream>

class TestSequence {
public:
    TestSequence(const std::string& val1_file)
        : val1_file_path(val1_file)
    {
        mp_val1_file = std::make_unique<std::ifstream>(val1_file_path);
    }

    TestSequence(const TestSequence& other)
        : val1_file_path(other.val1_file_path)
    {
        mp_val1_file = std::make_unique<std::ifstream>(val1_file_path);
    }

    TestSequence(TestSequence&& other) = default; // We want default one ;)
    // Other alternative implementation of above
    /*TestSequence(const TestSequence& other)
            : val1_file_path(other.val1_file_path)
        {
            mp_val1_file = std::make_unique<std::ifstream>(val1_file_path);
        }*/


    TestSequence& operator=(const TestSequence& other)
    {
        val1_file_path = other.val1_file_path;
        mp_val1_file = std::make_unique<std::ifstream>(val1_file_path);
        return *this;
    }

    TestSequence& operator=(TestSequence&& other) = default; 
    // Other alternative implementation of above
    /*TestSequence& operator=(TestSequence&& other) // move semantics from C++11
    {
        val1_file_path = std::move(other.val1_file_path);
        mp_val1_file = std::move(other.mp_val1_file);
        return *this;
    }*/

 private:
    std::string val1_file_path;
    std::unique_ptr<std::ifstream> mp_val1_file;
};

In this implementation I used smart pointers like: std::unique_ptr. To be sure I explicitly said that I want default move semantics operator= and move ctor (Maybe they will be generated by default but not sure so I prefer explicit mark that). It depends on your use case how you want to implement copy ctor and copy assign. In my case I reopen buffer, but maybe you want also copy position of buffer or other things. Simply by just creating new ifstream.

Other solution would be: (NOT RECOMENDED)

class TestSequence {
public:
    TestSequence(const std::string& val1_file)
        : val1_file_path(val1_file)
    {
        mp_val1_file = new std::ifstream(val1_file_path);
    }

    TestSequence(const TestSequence& other)
        : val1_file_path(other.val1_file_path)
    {
        mp_val1_file = new std::ifstream(val1_file_path);
    }

    ~TestSequence()
    {
        delete mp_val1_file;
        mp_val1_file = nullptr;
    }

    TestSequence& operator=(const TestSequence& other)
    {
        val1_file_path = other.val1_file_path;
        mp_val1_file = new std::ifstream(val1_file_path);
        return *this;
    }

 private:
    std::string val1_file_path;
    std::ifstream* mp_val1_file = nullptr;
};

Explanation to your code:

When you create instance of TestSequence

TestSequence mySequence("my/magic/path");
// mySequence = TestSequence{file_path="my/magic/path", input_stream=0x123123} (example)
// Now if you write such thing
TestSequence copy = mySequence; // same as TestSequence copy(mySequence);
// copy = TestSequence{file_path="my/magic/path", input_stream=0x123123} <--- same address because of default copy constructor

Now if mySequence will die eg. it was only a parameter of whatever, we call destructor so it will look like:

// mySequence = TestSequence{file_path="my/magic/path", input_stream=0x0}
// copy = TestSequence{file_path="my/magic/path", input_stream=0x123123}

So as you can see mySequence will free up your data under input_stream pointer but now when copy dies it will again try to free memory from input_stream which was already released.

What I can recommend to consider to don't keep ifstream as field if you don't need it. If you use it only to read test data, process it and check result. Consider opening file in this method/function. Try to keep life of such stream/variable as short as possible :)

Upvotes: 1

Related Questions