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

Conversation

Projects
None yet
5 participants
@endJunction
Copy link
Member

endJunction commented Nov 4, 2018

Move test definitions into the project files and add a "test mode" to the ogs executable.

@bilke

This comment has been minimized.

Copy link
Member

bilke commented Nov 5, 2018

Note to self:

  • Remove output
  • Globbing
@chleh
Copy link
Collaborator

chleh left a comment

Looks like a good feature: Easier to read than Tests.cmake. And can be extended without having to dig into CMake.

Some general comments:

  • Renaming of the reference files s.t. they match the solution files seems to be mandatory. That might be inconvenient if one develops a test case or if one wants to compare several time steps to an analytical solution.
  • There should be proper escaping for the system() calls. Otherwise, e.g., external people could hijack our CI servers by making a PR. OK, they can hijack them anyway, e.g., by adding something nasty to the cmake files. 😄
@@ -86,7 +86,7 @@ int main(int argc, char* argv[])
#else
"all",
#endif
"log level");

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

That's for the cmdline help text?
Maybe also PROJECT_FILE in L71?

std::string convert_to_string(double const& value)
{
// TODO (naumov) Replace this with fmt library.
char buffer[1024];

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

pretty much for a formatted double value. 😄

// TODO (naumov) Replace this with fmt library.
char buffer[1024];
int const chars_written = std::snprintf(buffer, 1024, "%g", value);
if (chars_written < 0 || chars_written > 1024)

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

Off by one: > 1023.
https://en.cppreference.com/w/cpp/io/c/fprintf:

  1. Number of characters that would have been written for a sufficiently large buffer if successful (not including the terminating null character), or a negative value if an error occurred. Thus, the (null-terminated) output has been completely written if and only if the returned value is nonnegative and less than buf_size.
/// Wraps a string into double ticks.
std::string safeString(std::string s)
{
return "\"" + s + "\"";

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

Only safe if all " in s are escaped.

//
int const return_value =
// TODO (naumov) replace system call with output consuming call.
// and extract the vtkdiff call to common function.

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

... and properly escape any strings occuring in command lines.

<test_definition>
<vtkdiff>
<file>cube_1e0_pcs_0_ts_1_t_0.000100.vtu</file>
<field>displacement</field>

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

As mentioned before: ref_field and field might be good.

@@ -12,6 +12,8 @@
<youngs_modulus>E</youngs_modulus>
<poissons_ratio>nu</poissons_ratio>
</constitutive_relation>
<solid_density>rho_sr</solid_density>
<specific_body_force>0 0 0</specific_body_force>

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

So, this was never tested before?

This comment has been minimized.

@endJunction

endJunction Nov 5, 2018

Author Member

LIS only, and there is no LIS build in CI ;(

endif()

set(OgsTest_SOURCE_DIR "${Data_SOURCE_DIR}/${OgsTest_DIR}")
set(OgsTest_BINARY_DIR "${Data_BINARY_DIR}/${OgsTest_DIR}")

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

Does it work if OGS is installed systemwide? Or if a test resides outside the source/build directory?
OK, the cmake stuff won't work without source/build dirs. But does the feature itself (ogs --reference ...) work? Or do you need to set some environment variables s.t. it works?

This comment has been minimized.

@endJunction

endJunction Nov 5, 2018

Author Member

Works as promised from anywhere. Only if vtkdiff is not in current PATH and not in ./bin the VTKDIFF_BINARY should be set...

@@ -61,10 +61,12 @@ function (AddTest)
set (AddTest_REQUIREMENTS TRUE)
endif()

set(X_PRJ_FILE ${AddTest_EXECUTABLE_ARGS}) # For easier retrival later on

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

Easier retrieval of what?

}
return return_value;
});
return all_of(begin(return_values), end(return_values),

This comment has been minimized.

@chleh

chleh Nov 5, 2018

Collaborator

IMHO the test should fail if return_values is empty (configuration error, no tests defined).

@bilke

This comment has been minimized.

Copy link
Member

bilke commented Nov 5, 2018

@endJunction @chleh Jenkins CI jobs are submitted for members of our GitHub ufz-organization only (https://github.com/orgs/ufz/teams/members/members).

@chleh

This comment has been minimized.

Copy link
Collaborator

chleh commented Nov 5, 2018

@bilke, great!

@endJunction endJunction force-pushed the endJunction:OgsTester branch from c434910 to f408ef5 Nov 6, 2018

@bilke

This comment has been minimized.

Copy link
Member

bilke commented Nov 9, 2018

@endJunction I guess for a clean implementation of file globbing we need either boost::filesystem or std::filesystem. Maybe this can be discussed and addressed later on.

@endJunction endJunction force-pushed the endJunction:OgsTester branch 3 times, most recently from 22e0855 to c5de2d6 Nov 12, 2018

@endJunction endJunction changed the title ogs tester. [Discussion] OgsTester. Add test definitions to project files. Nov 13, 2018

std::string const absolute_tolerance_parameter =
absolute_tolerance == boost::none
? ""
: "--abs " + convert_to_string(*absolute_tolerance);

This comment has been minimized.

@chleh

chleh Nov 13, 2018

Collaborator

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

@endJunction endJunction force-pushed the endJunction:OgsTester branch 4 times, most recently from 8710035 to 5c21155 Nov 13, 2018

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Nov 14, 2018

The conversion scripts, previously added in the AddTest.cmake are in the last commit on OgsTesterConversionScripts branch.

if (reference_path.empty())
{
OGS_FATAL(
"Reference path containting expected result files can not be "

This comment has been minimized.

@TomFischer

TomFischer Nov 14, 2018

Member

containting -> containing ✔️

@endJunction endJunction force-pushed the endJunction:OgsTester branch from 318569e to 0d5cb3a Nov 14, 2018

endJunction added some commits Nov 12, 2018

[App] Implement TestDefinition and test run.
Add --reference PATH option to ogs, which when present
will execute tests (vtkdiff) constructed from the
<test_definiton> section in the project file.
This section contains the tester (vtkdiff for now)
the file, field, and absolute and relative tolerances.

This allows the user to run tests w/o cmake and includes
the previously separated test description into the
project file.

@endJunction endJunction force-pushed the endJunction:OgsTester branch from 0d5cb3a to a589380 Nov 14, 2018

@TomFischer
Copy link
Member

TomFischer left a comment

Looks good.

@wenqing
Copy link
Member

wenqing left a comment

👍

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

This comment has been minimized.

@wenqing

wenqing Nov 14, 2018

Member

How about std::ostringstream?

This comment has been minimized.

@endJunction

endJunction Nov 14, 2018

Author Member

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

This comment has been minimized.

@wenqing

wenqing Nov 14, 2018

Member

The new changes look good.

//! \ogs_file_param{prj__test_definition__vtkdiff__absolute_tolerance}
vtkdiff_config.getConfigParameterOptional<double>(
"absolute_tolerance");
vtkdiff_config.getConfigParameter<std::string>("absolute_tolerance",

This comment has been minimized.

@wenqing

wenqing Nov 14, 2018

Member

avoided the conversion by this way, 👍.

@endJunction endJunction force-pushed the endJunction:OgsTester branch from e60f278 to 6bb4cdb Nov 14, 2018

[App] TD; Do not convert tolerances to double.
Pass the value directly instead and only check if the string
convertible to a double if not empty.

This avoids possible losses due to conversion.

@endJunction endJunction force-pushed the endJunction:OgsTester branch from 6bb4cdb to a0e987b Nov 14, 2018

@endJunction endJunction merged commit 8dd1ce4 into ufz:master Nov 14, 2018

2 of 3 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details

@endJunction endJunction deleted the endJunction:OgsTester branch Nov 14, 2018

@wenqing
Copy link
Member

wenqing left a comment

Just compiled with gnu compiler, got one warning and one error.

pos, s.c_str());
}

if (std::isnan(value))

This comment has been minimized.

@wenqing

wenqing Nov 16, 2018

Member

compilation error with g++: error: ‘isnan’ is not a member of ‘std’

This comment has been minimized.

@bilke

bilke Nov 16, 2018

Member

This has been fixed here 66eb63a

This comment has been minimized.

@wenqing
bool isConvertibleToDouble(std::string const& s)
{
std::size_t pos = 0;
double value;

This comment has been minimized.

@wenqing

wenqing Nov 16, 2018

Member

compilation warning: warning: variable ‘value’ set but not used.

This comment has been minimized.

@wenqing

wenqing Nov 20, 2018

Member

This warning is gone now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.