Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OgsTester. Add test definitions to project files. #2255

Merged
merged 12 commits into from
Nov 14, 2018
Merged
229 changes: 229 additions & 0 deletions Applications/ApplicationsLib/TestDefinition.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
/**
* \file
*
* \copyright
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)
* Distributed under a Modified BSD License.
* See accompanying file LICENSE.txt or
* http://www.opengeosys.org/project/license
*
*/

#include "TestDefinition.h"

#include <cstdlib>

#include "BaseLib/ConfigTree.h"
#include "BaseLib/Error.h"
#include "BaseLib/FileTools.h"
#ifdef USE_PETSC
#include "MeshLib/IO/VtkIO/VtuInterface.h" // For petsc file name conversion.
#include <petsc.h>
#endif

namespace
{
/// Safe conversion of a double to a string using decimal or decimal exponent
/// notation. See std::snprintf() for details for "%g" conversion specifier.
Copy link
Member

@wenqing wenqing Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about std::ostringstream?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Even better, I rewrote that part to avoid any conversion. Could you please check the changes in the last commit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new changes look good.

/// \note std::to_string uses "%f" conversion specifier which is not sufficient
/// for small numbers like 1e-15.
std::string convert_to_string(double const& value)
{
// TODO (naumov) Replace this with fmt library.
char buffer[32];
int const chars_written =
std::snprintf(buffer, sizeof(buffer), "%g", value);
if (chars_written < 0 || chars_written >= static_cast<int>(sizeof(buffer)))
{
OGS_FATAL("Could not convert a value to string.");
}
return std::string{buffer};
}

/// Wraps a string into double ticks.
std::string safeString(std::string s)
{
return "\"" + s + "\"";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only safe if all " in s are escaped.

}

/// Tries to find a vtkdiff executable by testing 'path/vtkdiff --version' calls
/// for various paths.
std::string findVtkdiff()
{
// Try to read the VTKDIFF_EXE environment variable.
if (const char* vtkdiff_exe_environment_variable =
std::getenv("VTKDIFF_EXE"))
{
std::string const vtkdiff_exe{vtkdiff_exe_environment_variable};
DBUG("VTKDIFF_EXE set to %s.", vtkdiff_exe.c_str());

//
// Sanity checks.
//
{ // The base name is 'vtkdiff'
auto const& base_name =
BaseLib::extractBaseNameWithoutExtension(vtkdiff_exe);
if (base_name != "vtkdiff")
{
OGS_FATAL(
"The VTKDIFF_EXE environment variable does not point to "
"'vtkdiff'. VTKDIFF_EXE='%s'",
vtkdiff_exe.c_str());
}
}
{ // vtkdiff must exist.
if (!BaseLib::IsFileExisting(vtkdiff_exe))
{
OGS_FATAL(
"The VTKDIFF_EXE points to a non-existing file. "
"VTKDIFF_EXE='%s'",
vtkdiff_exe.c_str());
}
}

//
// Test the actual call.
//
int const return_value =
// TODO (naumov) replace system call with output consuming call
// (fork + execl seems to be more safe), and extract the vtkdiff
// call to common function. Also properly escape all strings in
// command lines.
// Reference for POSIX and Windows:
// https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152177
// Take care when using fork, which might copy resources.
std::system((vtkdiff_exe + " --version").c_str());
if (return_value == 0)
{
return vtkdiff_exe;
}
WARN(
"Calling %s from the VTKDIFF_EXE environment variable didn't work "
"as expected. Return value was %d.",
vtkdiff_exe.c_str(), return_value);
}

std::string const vtkdiff_exe{"vtkdiff"};
std::vector<std::string> const paths = {"", "bin"};
auto const path = find_if(
begin(paths), end(paths), [&vtkdiff_exe](std::string const& path) {
int const return_value =
// TODO (naumov) replace system call with output consuming call
// as in an above todo comment.
std::system(
(BaseLib::joinPaths(path, vtkdiff_exe) + " --version")
.c_str());
return return_value == 0;
});
if (path == end(paths))
{
OGS_FATAL("vtkdiff not found.");
}
return BaseLib::joinPaths(*path, vtkdiff_exe);
}

} // namespace

namespace ApplicationsLib
{
TestDefinition::TestDefinition(BaseLib::ConfigTree const& config_tree,
std::string const& reference_path,
std::string const& output_directory)
{
if (reference_path.empty())
{
OGS_FATAL(
"Reference path containing expected result files can not be "
"empty.");
}

std::string const vtkdiff = findVtkdiff();

// Construct command lines for each entry.
//! \ogs_file_param{prj__test_definition__vtkdiff}
auto const& vtkdiff_configs = config_tree.getConfigSubtreeList("vtkdiff");
_command_lines.reserve(vtkdiff_configs.size());
for (auto const& vtkdiff_config : vtkdiff_configs)
{
std::string const& field_name =
//! \ogs_file_param{prj__test_definition__vtkdiff__field}
vtkdiff_config.getConfigParameter<std::string>("field");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So both the fields in the reference and OGSes results have to have the same names?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Until more is needed.

DBUG("vtkdiff will compare field '%s'.", field_name.c_str());

#ifdef USE_PETSC
int rank;
MPI_Comm_rank(PETSC_COMM_WORLD, &rank);
int mpi_size;
MPI_Comm_size(PETSC_COMM_WORLD, &mpi_size);
std::string const& filename =
MeshLib::IO::getVtuFileNameForPetscOutputWithoutExtension(
//! \ogs_file_param{prj__test_definition__vtkdiff__file}
vtkdiff_config.getConfigParameter<std::string>("file")) +
"_" + std::to_string(rank) + ".vtu";
#else
std::string const& filename =
//! \ogs_file_param{prj__test_definition__vtkdiff__file}
vtkdiff_config.getConfigParameter<std::string>("file");
#endif // OGS_USE_PETSC
std::string const& output_filename =
BaseLib::joinPaths(output_directory, filename);
_output_files.push_back(output_filename);
// TODO (naumov) expand filename relative to ref path for globbing.
std::string const& reference_filename =
BaseLib::joinPaths(reference_path, filename);

auto const& absolute_tolerance =
//! \ogs_file_param{prj__test_definition__vtkdiff__absolute_tolerance}
vtkdiff_config.getConfigParameterOptional<double>(
"absolute_tolerance");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't make that optional. If the tolerances are optional, some "arbitrary" default values might kick in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arbitrary value is 0; guess it's safe here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it or is it 1e-16? Who can tell from the prj file alone 😄

std::string const absolute_tolerance_parameter =
absolute_tolerance == boost::none
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== boost::none probably not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, just avoiding implicit bool conversion... or !!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?: is a boolean context. So it should be (implicitly) an explicit bool conversion.

? ""
: "--abs " + convert_to_string(*absolute_tolerance);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: the precedence of + over : is probably not working knowledge of everybody.


auto const& relative_tolerance =
//! \ogs_file_param{prj__test_definition__vtkdiff__relative_tolerance}
vtkdiff_config.getConfigParameterOptional<double>(
"relative_tolerance");
std::string const relative_tolerance_parameter =
relative_tolerance == boost::none
? ""
: "--rel " + convert_to_string(*relative_tolerance);

//
// Construct command line.
//
std::string command_line =
vtkdiff + " -a " + safeString(field_name) + " -b " +
safeString(field_name) + " " + safeString(reference_filename) +
" " + safeString(output_filename) + " " +
absolute_tolerance_parameter + " " + relative_tolerance_parameter;
INFO("Will run '%s'", command_line.c_str());
_command_lines.emplace_back(std::move(command_line));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving from a const string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

;( but the code looks so much more modern...

}
}

bool TestDefinition::runTests() const
{
std::vector<int> return_values;
transform(begin(_command_lines), end(_command_lines),
back_inserter(return_values),
[](std::string const& command_line) {
int const return_value = std::system(command_line.c_str());
if (return_value != 0)
{
WARN("Return value %d was returned by '%s'.",
return_value, command_line.c_str());
}
return return_value;
});
return !return_values.empty() &&
all_of(begin(return_values), end(return_values),
[](int const& return_value) { return return_value == 0; });
}

std::vector<std::string> const& TestDefinition::getOutputFiles() const
{
return _output_files;
}
} // namespace ApplicationsLib
42 changes: 42 additions & 0 deletions Applications/ApplicationsLib/TestDefinition.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* \file
*
* \copyright
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)
* Distributed under a Modified BSD License.
* See accompanying file LICENSE.txt or
* http://www.opengeosys.org/project/license
*
*/

#pragma once

#include <memory>
#include <string>
#include <vector>

namespace BaseLib
{
class ConfigTree;
}

namespace ApplicationsLib
{
class TestDefinition final
{
public:
/// Constructs test definition from the config and reference path
/// essentially constructing the command lines to be run on run() function
/// call.
TestDefinition(BaseLib::ConfigTree const& config_tree,
std::string const& reference_path,
std::string const& output_directory);

bool runTests() const;
std::vector<std::string> const& getOutputFiles() const;

private:
std::vector<std::string> _command_lines;
std::vector<std::string> _output_files;
};
} // namespace ApplicationsLib
49 changes: 48 additions & 1 deletion Applications/CLI/ogs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "Applications/ApplicationsLib/LinearSolverLibrarySetup.h"
#include "Applications/ApplicationsLib/LogogSetup.h"
#include "Applications/ApplicationsLib/ProjectData.h"
#include "Applications/ApplicationsLib/TestDefinition.h"
#include "Applications/InSituLib/Adaptor.h"
#include "ProcessLib/UncoupledProcessesTimeLoop.h"

Expand All @@ -63,6 +64,13 @@ int main(int argc, char* argv[])
' ',
BaseLib::BuildInfo::git_describe);

TCLAP::ValueArg<std::string> reference_path_arg(
"r", "reference",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't add a single-letter shortcut.

"Run output result comparison after successful simulation comparing to "
"all files in the given path.",
false, "", "PATH");
cmd.add(reference_path_arg);

TCLAP::UnlabeledValueArg<std::string> project_arg(
"project-file",
"Path to the ogs6 project file.",
Expand Down Expand Up @@ -141,6 +149,7 @@ int main(int argc, char* argv[])
INFO("OGS started on %s.", time_str.c_str());
}

std::unique_ptr<ApplicationsLib::TestDefinition> test_definition;
auto ogs_status = EXIT_SUCCESS;

try
Expand Down Expand Up @@ -173,6 +182,22 @@ int main(int argc, char* argv[])
BaseLib::getProjectDirectory(),
outdir_arg.getValue());

if (!reference_path_arg.isSet())
{ // Ignore the test_definition section.
project_config->ignoreConfigParameter("test_definition");
}
else
{
test_definition =
std::make_unique<ApplicationsLib::TestDefinition>(
//! \ogs_file_param{prj__test_definition}
project_config->getConfigSubtree("test_definition"),
reference_path_arg.getValue(),
outdir_arg.getValue());

INFO("Cleanup possible output files before running ogs.");
BaseLib::removeFiles(test_definition->getOutputFiles());
}
#ifdef USE_INSITU
auto isInsituConfigured = false;
//! \ogs_file_param{prj__insitu}
Expand Down Expand Up @@ -236,5 +261,27 @@ int main(int argc, char* argv[])
INFO("OGS terminated on %s.", time_str.c_str());
}

return ogs_status;
if (ogs_status == EXIT_FAILURE)
{
ERR("OGS terminated with error.");
return EXIT_FAILURE;
}

if (test_definition == nullptr)
{
// There are no tests, so just exit;
return ogs_status;
}

INFO("");
INFO("##########################################");
INFO("# Running tests #");
INFO("##########################################");
INFO("");
if (!test_definition->runTests())
{
ERR("One of the tests failed.");
return EXIT_FAILURE;
}
return EXIT_SUCCESS;
}
2 changes: 2 additions & 0 deletions BaseLib/ConfigTree-impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class Range

Iterator begin() const { return _begin; }
Iterator end() const { return _end; }
std::size_t size() const { return std::distance(_begin, _end); }

private:
Iterator _begin;
Iterator _end;
Expand Down
22 changes: 22 additions & 0 deletions BaseLib/FileTools.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,26 @@ void setProjectDirectory(std::string const& dir)
project_directory_is_set = true;
}

void removeFiles(std::vector<std::string> const& files)
{
for (auto const& file : files)
{
int const success = std::remove(file.c_str());
if (success == 0)
{
DBUG("Removed '%s'", file.c_str());
}
else
{
if (errno == ENOENT) // File does not exists
{
continue;
}
ERR("Removing file '%s' failed with error %d.", file.c_str(),
errno);
std::perror("Error: ");
OGS_FATAL("Unrecoverable error happened while removing a file.");
}
}
}
} // end namespace BaseLib
5 changes: 4 additions & 1 deletion BaseLib/FileTools.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,7 @@ std::string const& getProjectDirectory();
/// Sets the project directory.
void setProjectDirectory(std::string const& dir);

} // end namespace BaseLib
/// Remove files. If a file does not exist nothing will happen, other errors
/// lead to OGS_FATAL call.
void removeFiles(std::vector<std::string> const& files);
} // end namespace BaseLib
Loading