Skip to content

Commit

Permalink
TestSuite: pimpl Compare::*File* internals.
Browse files Browse the repository at this point in the history 
To avoid including a String -- only a StringView is needed now; plus in
case some further state is needed, it's much cheaper to add.
Theoretically, to further reduce the headers, there could be const char*
overloads, but since majority of cases involve using Path::join() anyway
(which, ahem, returns a String, which then the user has to include
anyway), I don't see a point right now.
  • Loading branch information
@mosra
mosra committed Mar 8, 2022
1 parent 7c327a7 commit 2870b77
Show file tree
Hide file tree
Showing 11 changed files with 168 additions and 102 deletions.
1 change: 1 addition & 0 deletions doc/snippets/TestSuite.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

#include "Corrade/Containers/Optional.h"
#include "Corrade/Containers/Pointer.h"
#include "Corrade/Containers/String.h"
#include "Corrade/TestSuite/Compare/File.h"
#include "Corrade/TestSuite/Compare/FileToString.h"
#include "Corrade/TestSuite/Compare/Numeric.h"
Expand Down
76 changes: 51 additions & 25 deletions src/Corrade/TestSuite/Compare/File.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,75 +31,101 @@
#include "Corrade/Containers/ArrayView.h"
#include "Corrade/Containers/Optional.h"
#include "Corrade/Containers/Pair.h"
#include "Corrade/Containers/String.h"
#include "Corrade/TestSuite/Comparator.h"
#include "Corrade/Utility/Math.h"
#include "Corrade/Utility/Path.h"

namespace Corrade { namespace TestSuite {

namespace {

enum class Result {
Success,
ReadError
};

}

struct Comparator<Compare::File>::State {
Result actualResult, expectedResult;
/* The whole comparison is done in a single expression so the path prefix
can stay as a view. However the filenames are join()ed with it, so they
have to be owned, same for contents fetched from the files. */
Containers::StringView pathPrefix;
Containers::String actualFilename, expectedFilename,
actualContents, expectedContents;
};

#ifndef DOXYGEN_GENERATING_OUTPUT
Comparator<Compare::File>::Comparator(const Containers::StringView pathPrefix): _actualState{State::ReadError}, _expectedState{State::ReadError}, _pathPrefix{pathPrefix} {}
Comparator<Compare::File>::Comparator(const Containers::StringView pathPrefix): _state{InPlaceInit} {
_state->actualResult = Result::ReadError;
_state->expectedResult = Result::ReadError;
_state->pathPrefix = pathPrefix;
}

Comparator<Compare::File>::~Comparator() = default;

ComparisonStatusFlags Comparator<Compare::File>::operator()(const Containers::StringView actualFilename, const Containers::StringView expectedFilename) {
_actualFilename = Utility::Path::join(_pathPrefix, actualFilename);
_expectedFilename = Utility::Path::join(_pathPrefix, expectedFilename);
_state->actualFilename = Utility::Path::join(_state->pathPrefix, actualFilename);
_state->expectedFilename = Utility::Path::join(_state->pathPrefix, expectedFilename);

/* Read the actual file contents before the expected so if the expected
file can't be read, we can still save actual file contents */
Containers::Optional<Containers::String> actualContents = Utility::Path::readString(_actualFilename);
Containers::Optional<Containers::String> actualContents = Utility::Path::readString(_state->actualFilename);
if(!actualContents)
return ComparisonStatusFlag::Failed;

_actualContents = *Utility::move(actualContents);
_actualState = State::Success;
_state->actualContents = *Utility::move(actualContents);
_state->actualResult = Result::Success;

/* If this fails, we already have the actual contents so we can save them */
Containers::Optional<Containers::String> expectedContents = Utility::Path::readString(_expectedFilename);
Containers::Optional<Containers::String> expectedContents = Utility::Path::readString(_state->expectedFilename);
if(!expectedContents)
return ComparisonStatusFlag::Diagnostic|ComparisonStatusFlag::Failed;

_expectedContents = *Utility::move(expectedContents);
_expectedState = State::Success;
_state->expectedContents = *Utility::move(expectedContents);
_state->expectedResult = Result::Success;

return _actualContents == _expectedContents ? ComparisonStatusFlags{} :
return _state->actualContents == _state->expectedContents ? ComparisonStatusFlags{} :
ComparisonStatusFlag::Diagnostic|ComparisonStatusFlag::Failed;
}

void Comparator<Compare::File>::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* actual, const char* expected) const {
if(_actualState != State::Success) {
out << "Actual file" << actual << "(" + _actualFilename + ")" << "cannot be read.";
if(_state->actualResult != Result::Success) {
out << "Actual file" << actual << "(" + _state->actualFilename + ")" << "cannot be read.";
return;
}

if(_expectedState != State::Success) {
out << "Expected file" << expected << "(" + _expectedFilename + ")" << "cannot be read.";
if(_state->expectedResult != Result::Success) {
out << "Expected file" << expected << "(" + _state->expectedFilename + ")" << "cannot be read.";
return;
}

out << "Files" << actual << "and" << expected << "have different";
if(_actualContents.size() != _expectedContents.size())
out << "size, actual" << _actualContents.size() << "but" << _expectedContents.size() << "expected.";
if(_state->actualContents.size() != _state->expectedContents.size())
out << "size, actual" << _state->actualContents.size() << "but" << _state->expectedContents.size() << "expected.";
else
out << "contents.";

for(std::size_t i = 0, end = Utility::max(_actualContents.size(), _expectedContents.size()); i != end; ++i) {
if(_actualContents.size() > i && _expectedContents.size() > i && _actualContents[i] == _expectedContents[i]) continue;
for(std::size_t i = 0, end = Utility::max(_state->actualContents.size(), _state->expectedContents.size()); i != end; ++i) {
if(_state->actualContents.size() > i && _state->expectedContents.size() > i && _state->actualContents[i] == _state->expectedContents[i]) continue;

if(_actualContents.size() <= i)
out << "Expected has character" << _expectedContents.slice(i, i + 1);
else if(_expectedContents.size() <= i)
out << "Actual has character" << _actualContents.slice(i, i + 1);
if(_state->actualContents.size() <= i)
out << "Expected has character" << _state->expectedContents.slice(i, i + 1);
else if(_state->expectedContents.size() <= i)
out << "Actual has character" << _state->actualContents.slice(i, i + 1);
else
out << "Actual character" << _actualContents.slice(i, i + 1) << "but" << _expectedContents.slice(i, i + 1) << "expected";
out << "Actual character" << _state->actualContents.slice(i, i + 1) << "but" << _state->expectedContents.slice(i, i + 1) << "expected";

out << "on position" << i << Utility::Debug::nospace << ".";
break;
}
}

void Comparator<Compare::File>::saveDiagnostic(ComparisonStatusFlags, Utility::Debug& out, const Containers::StringView path) {
Containers::String filename = Utility::Path::join(path, Utility::Path::split(_expectedFilename).second());
if(Utility::Path::write(filename, _actualContents))
Containers::String filename = Utility::Path::join(path, Utility::Path::split(_state->expectedFilename).second());
if(Utility::Path::write(filename, _state->actualContents))
out << "->" << filename;
}
#endif
Expand Down
23 changes: 9 additions & 14 deletions src/Corrade/TestSuite/Compare/File.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@
* @brief Class @ref Corrade::TestSuite::Compare::File
*/

#include "Corrade/Containers/String.h"
#include "Corrade/Containers/Pointer.h"
/* The include is not strictly needed, but it would only mean the users would
then have to include it on their own -- as there's no way to use this
comparator without a StringView */
#include "Corrade/Containers/StringView.h"
#include "Corrade/TestSuite/TestSuite.h"
#include "Corrade/TestSuite/visibility.h"
#include "Corrade/Utility/Utility.h"
Expand All @@ -50,26 +54,17 @@ template<> class CORRADE_TESTSUITE_EXPORT Comparator<Compare::File> {
public:
explicit Comparator(Containers::StringView pathPrefix = {});

~Comparator();

ComparisonStatusFlags operator()(Containers::StringView actualFilename, Containers::StringView expectedFilename);

void printMessage(ComparisonStatusFlags flags, Utility::Debug& out, const char* actual, const char* expected) const;

void saveDiagnostic(ComparisonStatusFlags flags, Utility::Debug& out, Containers::StringView path);

private:
enum class State {
Success,
ReadError
};

State _actualState, _expectedState;
/* The whole comparison is done in a single expression so the path
prefix can stay as a view. However the filenames are join()ed with
it, so they have to be owned, same for contents fetched from the
files. */
Containers::StringView _pathPrefix;
Containers::String _actualFilename, _expectedFilename,
_actualContents, _expectedContents;
struct CORRADE_TESTSUITE_LOCAL State;
Containers::Pointer<State> _state;
};
#endif

Expand Down
58 changes: 41 additions & 17 deletions src/Corrade/TestSuite/Compare/FileToString.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,50 +29,74 @@
#include <cstddef>

#include "Corrade/Containers/Optional.h"
#include "Corrade/Containers/String.h"
#include "Corrade/TestSuite/Comparator.h"
#include "Corrade/Utility/Math.h"
#include "Corrade/Utility/Path.h"

namespace Corrade { namespace TestSuite {

Comparator<Compare::FileToString>::Comparator(): _state(State::ReadError) {}
namespace {

enum class Result {
Success,
ReadError
};

}

struct Comparator<Compare::FileToString>::State {
Result result;
/* The whole comparison is done in a single expression so the filename and
expected contents can stay as views, however actual contents are fetched
from a file so they have be owned */
Containers::StringView filename;
Containers::String actualContents;
Containers::StringView expectedContents;
};

Comparator<Compare::FileToString>::Comparator(): _state{InPlaceInit} {
_state->result = Result::ReadError;
}

Comparator<Compare::FileToString>::~Comparator() = default;

ComparisonStatusFlags Comparator<Compare::FileToString>::operator()(const Containers::StringView filename, const Containers::StringView expectedContents) {
_filename = filename;
_state->filename = filename;

Containers::Optional<Containers::String> actualContents = Utility::Path::readString(filename);
if(!actualContents)
return ComparisonStatusFlag::Failed;

_actualContents = *Utility::move(actualContents);
_expectedContents = expectedContents;
_state = State::Success;
_state->actualContents = *Utility::move(actualContents);
_state->expectedContents = expectedContents;
_state->result = Result::Success;

return _actualContents == expectedContents ? ComparisonStatusFlags{} :
return _state->actualContents == expectedContents ? ComparisonStatusFlags{} :
ComparisonStatusFlag::Failed;
}

void Comparator<Compare::FileToString>::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* actual, const char* expected) const {
if(_state != State::Success) {
out << "File" << actual << "(" + _filename + ")" << "cannot be read.";
if(_state->result != Result::Success) {
out << "File" << actual << "(" + _state->filename + ")" << "cannot be read.";
return;
}

out << "Files" << actual << "and" << expected << "have different";
if(_actualContents.size() != _expectedContents.size())
out << "size, actual" << _actualContents.size() << "but" << _expectedContents.size() << "expected.";
if(_state->actualContents.size() != _state->expectedContents.size())
out << "size, actual" << _state->actualContents.size() << "but" << _state->expectedContents.size() << "expected.";
else
out << "contents.";

for(std::size_t i = 0, end = Utility::max(_actualContents.size(), _expectedContents.size()); i != end; ++i) {
if(_actualContents.size() > i && _expectedContents.size() > i && _actualContents[i] == _expectedContents[i]) continue;
for(std::size_t i = 0, end = Utility::max(_state->actualContents.size(), _state->expectedContents.size()); i != end; ++i) {
if(_state->actualContents.size() > i && _state->expectedContents.size() > i && _state->actualContents[i] == _state->expectedContents[i]) continue;

if(_actualContents.size() <= i)
out << "Expected has character" << _expectedContents.slice(i, i + 1);
else if(_expectedContents.size() <= i)
out << "Actual has character" << _actualContents.slice(i, i + 1);
if(_state->actualContents.size() <= i)
out << "Expected has character" << _state->expectedContents.slice(i, i + 1);
else if(_state->expectedContents.size() <= i)
out << "Actual has character" << _state->actualContents.slice(i, i + 1);
else
out << "Actual character" << _actualContents.slice(i, i + 1) << "but" << _expectedContents.slice(i, i + 1) << "expected";
out << "Actual character" << _state->actualContents.slice(i, i + 1) << "but" << _state->expectedContents.slice(i, i + 1) << "expected";

out << "on position" << i << Utility::Debug::nospace << ".";
break;
Expand Down
24 changes: 10 additions & 14 deletions src/Corrade/TestSuite/Compare/FileToString.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@
* @brief Class @ref Corrade::TestSuite::Compare::FileToString
*/

#include "Corrade/Containers/String.h"
#include "Corrade/Containers/Pointer.h"
/* The include is not strictly needed, but it would only mean the users would
then have to include it on their own -- as there's no way to use this
comparator without a StringView */
#include "Corrade/Containers/StringView.h"
#include "Corrade/TestSuite/TestSuite.h"
#include "Corrade/TestSuite/visibility.h"
#include "Corrade/Utility/Utility.h"
Expand Down Expand Up @@ -70,25 +74,17 @@ class FileToString {};
#ifndef DOXYGEN_GENERATING_OUTPUT
template<> class CORRADE_TESTSUITE_EXPORT Comparator<Compare::FileToString> {
public:
Comparator();
explicit Comparator();

~Comparator();

ComparisonStatusFlags operator()(Containers::StringView filename, Containers::StringView expectedContents);

void printMessage(ComparisonStatusFlags flags, Utility::Debug& out, const char* actual, const char* expected) const;

private:
enum class State {
Success,
ReadError
};

State _state;
/* The whole comparison is done in a single expression so the filename
and expected contents can stay as views, however actual contents are
fetched from a file so they have be owned */
Containers::StringView _filename;
Containers::String _actualContents;
Containers::StringView _expectedContents;
struct CORRADE_TESTSUITE_LOCAL State;
Containers::Pointer<State> _state;
};
#endif

Expand Down

0 comments on commit 2870b77

Please sign in to comment.