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

Mark skipped SLI tests as skipped, not success #713

Merged
merged 7 commits into from May 27, 2017

Conversation

@hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Apr 18, 2017

This PR fixes #359, and contains the following

  • When NEST is compiled with MPI, tests that require NEST to be compiled without MPI are skipped with exit_test_gracefully instead of calling quit_i directly.
  • Making exit_test_gracefully mark the test as skipped, not success.
  • A new exit code, 200, has been introduced to mark skipped tests.
@@ -538,6 +539,7 @@ SLIStartup::init( SLIInterpreter* i )

exitcodes->insert(
exitcode_success_name, Token( new IntegerDatum( EXIT_SUCCESS ) ) );
exitcodes->insert( exitcode_skipped_name, Token( new IntegerDatum( 200 ) ) );

This comment has been minimized.

@gtrensch

gtrensch Apr 27, 2017
Collaborator

In general it is not good practice to have raw numbers in the code. Named constants should be used instead.

@@ -412,6 +420,8 @@ echo > "${TEST_LOGFILE}" "NEST v. @NEST_VERSION_VERSION@ testsuite log"
echo >> "${TEST_LOGFILE}" "======================"
echo >> "${TEST_LOGFILE}" "Running tests from ${TEST_BASEDIR}"

CODES_SKIPPED=' 200 Skipped'

This comment has been minimized.

@gtrensch

gtrensch Apr 27, 2017
Collaborator

I'm not sure about that but one may ask why these tests are skipped. CODES_SKIPPED=' 200 Skipped (build with-mpi=OFF required)' or something like that would make it clear. We have similar comments at other places, e.g. Success (expected failure).

@gtrensch
Copy link
Collaborator

@gtrensch gtrensch commented Apr 27, 2017

I tested the build with and without MPI. Everything works as expected. 👍 . I would approve this PR, but please see also my comments.

@hakonsbm
Copy link
Contributor Author

@hakonsbm hakonsbm commented Apr 28, 2017

@gtrensch I have now used named constants for the exit codes. Are they in the right place? And should I also add named constants for the other exit codes (scripterror, exception, etc)?

There are quite a few new changed files now, but most of the new changes are minor changes to tests. This is because in order to tell do_tests.sh why a test is skipped, I have introduced some new exit codes and a new SLI function, exit_test_gracefully_reason[/stringtype]. This way do_tests.sh can differentiate different reasons for skipping, and give associated comments.

@gtrensch
Copy link
Collaborator

@gtrensch gtrensch commented Apr 29, 2017

@hakonsbm many thanks for the work ! I think the constants are in the right place. Replacing all the other exit codes would of course be a good idea. But I'm not sure if this PR is the right place to do this refactoring task.

Copy link
Contributor

@heplesser heplesser left a comment

@hakonsbm Thanks for your effort, this is an important step forward! I added some suggestions for further polishing.

}
if
is_threaded not {(no_openmp) exit_test_gracefully_reason} if
statusdict/have_gsl :: not {(no_gsl) exit_test_gracefully_reason} if

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

Add spaces inside the curly braces.

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

We have a small number of skipping conditions which lead to a lot of repeated code. I think we could make things much tidier by introducing functions for each of the cases, e.g.,

/skip_if_not_threaded { is_threaded not { /no_openmp exit_test_gracefully } if } def

Then, in this case, the test code would just read

skip_if_not_threaded
skip_if_without_gsl
}
if
is_threaded not {(no_openmp) exit_test_gracefully_reason} if
statusdict/have_gsl :: not {(no_gsl) exit_test_gracefully_reason} if

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

Add spaces inside the curly braces.

@@ -42,7 +42,7 @@

M_ERROR setverbosity

is_threaded_with_openmp not { exit_test_gracefully } if
is_threaded_with_openmp not { (no_openmp) exit_test_gracefully_reason } if

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

I think we can simplify here to is_threaded and remove the is_threaded_with_openmp. OpenMP has been the only threading option for a long while now.

dup (no_music) eq {statusdict/exitcodes/skipped_no_music :: quit_i} if
statusdict/exitcodes/scripterror :: quit_i
} bind def

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

I think this can become a bit tidier:

  • Drop the old exit_test_gracefully and drop the ..._reason from this version.
  • Change the function to take a /literaltype argument, e.g. /skipped_no_mpi. Then the function simplifies to
{ statusdict/exitcodes :: exch get quit_i }
  • If the argument is not found in the dictionary, SLI will fail with a scripterror automatically in this case.
  • To cover the "unspecified" case, just call /skipped exit_test_gracefully

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

I would also change all "openmp" into "no_threads" or "not_threaded". OpenMP is an implementation detail.

Token( new IntegerDatum( NEST_EXITCODE_SKIPPED_NO_GSL ) ) );
exitcodes->insert( exitcode_skipped_no_music_name,
Token( new IntegerDatum( NEST_EXITCODE_SKIPPED_NO_MUSIC ) ) );
exitcodes->insert(
exitcode_scripterror_name, Token( new IntegerDatum( 126 ) ) );

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

To be systematic, turn this and the other numeric values into preprocessor constants, too.

#define NEST_EXITCODE_SKIPPED_HAVE_MPI 202
#define NEST_EXITCODE_SKIPPED_NO_OPENMP 203
#define NEST_EXITCODE_SKIPPED_NO_GSL 204
#define NEST_EXITCODE_SKIPPED_NO_MUSIC 205

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

Add the other numeric values from slistartup.cpp here (in increasing order). These exit codes are not related to NEST as such, so drop NEST_ from the names. Maybe add a comment that the exit code range 200-215 is reserved for test skipping exits, and point out that any new skipping codes must be added to testsuite/do_tests.sh.in.

@@ -249,7 +249,8 @@ run_test ()

param_script="$1"
param_success="$2"
param_failure="$3"
param_skipped="$3"
param_failure="$4"

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

Please update the documentation of run_test() above to include param_skipped.

TEST_PASSED=$(( ${TEST_PASSED} + 1 ))
msg_dirty=${param_skipped##* ${exit_code} }
msg_clean=${msg_dirty%%,*}
explanation="${msg_clean}"

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

If I understand this code right, then this section handles skipped tests. The Travis log shows that correct messages are shown for individual skipped tests, but the skipped tests should be counted as skipped, not as passed, and the final summary should show them as skipped.

msg_clean=${msg_dirty%%,*}
if test "${msg_dirty}" != "${param_success}" ; then
TEST_PASSED=$(( ${TEST_PASSED} + 1 ))
explanation="${msg_clean}"
junit_failure=
elif test "${msg_dirty_skip}" != "${param_skipped}" ; then
TEST_PASSED=$(( ${TEST_PASSED} + 1 ))
msg_dirty=${param_skipped##* ${exit_code} }

This comment has been minimized.

@heplesser

heplesser May 24, 2017
Contributor

The right hand side here is the same as on line 292, maybe no need to repeat?

@heplesser heplesser merged commit 15b63f2 into nest:master May 27, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hakonsbm hakonsbm deleted the hakonsbm:skipped_sli_tests branch May 29, 2017
@hakonsbm hakonsbm mentioned this pull request Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.