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

Add external ringtest to CTest suite #997

Merged
merged 10 commits into from
Feb 19, 2021
Merged

Conversation

olupton
Copy link
Collaborator

@olupton olupton commented Feb 16, 2021

The addition in this PR is the set of nrn_add_test*(...) CMake helpers, which are used to configure integration tests from https://github.com/neuronsimulator/ringtest as part of the CTest suite. These helpers are documented in cmake/NeuronTestHelper.cmake. Note that, because of small numerical differences when TABLE statements are removed for CoreNEURON compatibility, the ringtest tests are not run if neither CoreNEURON nor NRN_ENABLE_MOD_COMPATIBILITY is enabled.

This PR also enables the CoreNEURON CTest suite if the NEURON CTests are enabled and CoreNEURON is built as a submodule. This means all the tests can be run with a single make test in this configuration, which is likely to be a sensible setup for developers working on NEURON and CoreNEURON.

Minor other changes:

  • Replace which with command -v to suppress a rather verbose message (/usr/bin/which: no xcrun in (...long list of paths...))
  • cmake-format the remainder of test/CMakeLists.txt as many lines were already touched in this PR.
  • Allow code coverage jobs to run in parallel (i.e. make test ARGS='-j 8'), previously there was a data race due to multiple jobs writing the same output file.

This suppresses a rather verbose message:
 /usr/bin/which: no xcrun in (...long list of paths...)
This is conditional on the NEURON tests being enabled. This is likely to
be a sensible setup for developers working on NEURON and CoreNEURON.
Previously multiple jobs would try to write to the same `.coverage`
file, causing a data race and job failures if the tests were run in
parallel. This forces each job to write to a separate output file.
Add several CMake helpers (nrn_add_test_group, nrn_add_test,
nrn_add_test_group_comparison) and use them to integrate tests from the
external neuronsimulator/ringtest repository into the CTest suite of
NEURON. NEURON and CoreNEURON are run in several configurations and the
results are compared against each other and a reference file. Note that,
because of small numerical differences when TABLE statements are removed
for CoreNEURON compatibility, the tests are not run if neither
CoreNEURON nor NRN_ENABLE_MOD_COMPATIBILITY is enabled.
Many lines in this file had already been touched in recent commits.
@olupton olupton requested a review from pramodk February 16, 2021 12:41
- Support older (v3.8.2) CMake versions.
- Fix a trivial error in a coverage report filename, fixing parallel
  runs of some tests.
- Add more workarounds for issues with Python in MPI tests, which seem
  to be related to #894.
- Fix dependencies of test targets.
- Disable {Python, MPI} tests when those features are disabled.
- Use the CORENEURONLIB environment variable so tests run reliably from
  working directories that are *not* where `nrnivmodl` was run.
- Make {build_dir}/bin/nrnivmodl executable, previously it was only made
  executable when it was installed.
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Very Nice! 👌

I have few comments. We can discuss some of the details offline.

bin/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Show resolved Hide resolved
test/CMakeLists.txt Show resolved Hide resolved
test/external/CMakeLists.txt Show resolved Hide resolved
test/external/CMakeLists.txt Show resolved Hide resolved
cmake/NeuronTestHelper.cmake Show resolved Hide resolved
cmake/NeuronTestHelper.cmake Show resolved Hide resolved
cmake/NeuronTestHelper.cmake Outdated Show resolved Hide resolved
cmake/NeuronTestHelper.cmake Show resolved Hide resolved
cmake/NeuronTestHelper.cmake Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 18, 2021

Codecov Report

Merging #997 (bd596a3) into master (bc751a1) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #997      +/-   ##
==========================================
+ Coverage   27.31%   27.37%   +0.05%     
==========================================
  Files         574      574              
  Lines      109074   109072       -2     
==========================================
+ Hits        29790    29854      +64     
+ Misses      79284    79218      -66     
Impacted Files Coverage Δ
src/parallel/bbssrvmpi.cpp 13.92% <0.00%> (-29.12%) ⬇️
src/parallel/bbsclimpi.cpp 29.87% <0.00%> (-21.43%) ⬇️
src/nrnmpi/bbsmpipack.cpp 74.01% <0.00%> (-12.43%) ⬇️
src/parallel/bbssrv2mpi.cpp 51.14% <0.00%> (-0.58%) ⬇️
src/parallel/bbs.cpp 62.91% <0.00%> (ø)
src/oc/hoc_oop.cpp 57.33% <0.00%> (+0.08%) ⬆️
src/nrncvode/netcvode.cpp 33.37% <0.00%> (+0.11%) ⬆️
src/oc/code.cpp 61.06% <0.00%> (+0.14%) ⬆️
src/nrnoc/solve.cpp 81.16% <0.00%> (+0.27%) ⬆️
src/nrnpython/nrnpy_hoc.cpp 52.72% <0.00%> (+0.36%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc751a1...bd596a3. Read the comment docs.

test/CMakeLists.txt Outdated Show resolved Hide resolved
test/external/CMakeLists.txt Outdated Show resolved Hide resolved
- Remove some unnecessary $ENV{SHELL} now nrnivmodl is executable in the
  build directory.
- Reorganise some comments.
Copy link
Member

@alexsavulescu alexsavulescu left a comment

Choose a reason for hiding this comment

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

LGTM, some nitpicks.

test/CMakeLists.txt Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/scripts/compare_test_results.py Outdated Show resolved Hide resolved
test/scripts/compare_test_results.py Outdated Show resolved Hide resolved
Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

Looks great from a user perspective. I'm getting

100% tests passed, 0 tests failed out of 38

Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

LGTM

Remove extraneous Python imports and tweak a comment.
@pramodk pramodk merged commit 0308ab8 into master Feb 19, 2021
@pramodk pramodk deleted the olupton_integration_ctests branch February 19, 2021 10:11
alexsavulescu pushed a commit that referenced this pull request Apr 13, 2021
* Add external `ringtest` to CTest suite.
    - Add several CMake helpers (nrn_add_test_group, nrn_add_test,
    nrn_add_test_group_comparison) and use them to integrate tests from the
    external neuronsimulator/ringtest repository into the CTest suite of
    NEURON.
    - NEURON and CoreNEURON are run in several configurations and the
    results are compared against each other and a reference file. Note that,
    because of small numerical differences when TABLE statements are removed
    for CoreNEURON compatibility, the tests are not run if neither CoreNEURON
    nor NRN_ENABLE_MOD_COMPATIBILITY is enabled.
* Replace `which` with `command -v`.
   This suppresses a rather verbose message:
    /usr/bin/which: no xcrun in (...long list of paths...)
* Run CoreNEURON tests if it is built as a submodule.
    This is conditional on the NEURON tests being enabled. This is likely to
     be a sensible setup for developers working on NEURON and CoreNEURON.
* Allow code coverage jobs to run in parallel.
   Previously multiple jobs would try to write to the same `.coverage`
   file, causing a data race and job failures if the tests were run in
   parallel. This forces each job to write to a separate output file.
* cmake-format the remainder of test/CMakeLists.txt
alexsavulescu pushed a commit that referenced this pull request Apr 30, 2021
* Add external `ringtest` to CTest suite.
    - Add several CMake helpers (nrn_add_test_group, nrn_add_test,
    nrn_add_test_group_comparison) and use them to integrate tests from the
    external neuronsimulator/ringtest repository into the CTest suite of
    NEURON.
    - NEURON and CoreNEURON are run in several configurations and the
    results are compared against each other and a reference file. Note that,
    because of small numerical differences when TABLE statements are removed
    for CoreNEURON compatibility, the tests are not run if neither CoreNEURON
    nor NRN_ENABLE_MOD_COMPATIBILITY is enabled.
* Replace `which` with `command -v`.
   This suppresses a rather verbose message:
    /usr/bin/which: no xcrun in (...long list of paths...)
* Run CoreNEURON tests if it is built as a submodule.
    This is conditional on the NEURON tests being enabled. This is likely to
     be a sensible setup for developers working on NEURON and CoreNEURON.
* Allow code coverage jobs to run in parallel.
   Previously multiple jobs would try to write to the same `.coverage`
   file, causing a data race and job failures if the tests were run in
   parallel. This forces each job to write to a separate output file.
* cmake-format the remainder of test/CMakeLists.txt
@alexsavulescu alexsavulescu mentioned this pull request Mar 22, 2022
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants