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

Topology tests into installcheck #705

Merged
merged 8 commits into from Jun 30, 2017

Conversation

Projects
None yet
4 participants
@hakonsbm
Contributor

hakonsbm commented Apr 7, 2017

This PR

  • Converts manual tests in topology to MPI and unittests to be used with installcheck.
  • Converts all SLI topology connection tests to topology unittests with reference results in each file.
  • Changes the installcheck script to include the topology SLI and Python tests.

This addresses parts of #302.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 7, 2017

Contributor

@jougs Could you mainly check the technical side, i.e., installation and running of tests?
@jhnnsnk Could you mainly look at the tests themselves? They have all been around for a long time, but just as manual tests.

Don't be scared by the seemingly very large number of changed files. This is mostly because files have been moved and reference data that used to be in three data files per test are now included in the actual test files. So most of the file changes are just deletions.

Contributor

heplesser commented Apr 7, 2017

@jougs Could you mainly check the technical side, i.e., installation and running of tests?
@jhnnsnk Could you mainly look at the tests themselves? They have all been around for a long time, but just as manual tests.

Don't be scared by the seemingly very large number of changed files. This is mostly because files have been moved and reference data that used to be in three data files per test are now included in the actual test files. So most of the file changes are just deletions.

@jougs

jougs approved these changes Apr 25, 2017

👍 for the technical side.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 1, 2017

Contributor

@hakonsbm Could you update this PR to resolve the conflict with master?

Contributor

heplesser commented Jun 1, 2017

@hakonsbm Could you update this PR to resolve the conflict with master?

@hakonsbm

This comment has been minimized.

Show comment
Hide comment
@hakonsbm

hakonsbm Jun 2, 2017

Contributor

@heplesser Updated.

Contributor

hakonsbm commented Jun 2, 2017

@heplesser Updated.

@heplesser heplesser added this to the NEST 2.12.1 milestone Jun 29, 2017

@jhnnsnk

Thanks for this work. I have some comments, mainly on consistency:

  • There are many tests for masks, but I miss tests for other functionality, e.g., kernels, parameters, prescribed number of connections, multapses/autapses, visualization functions...
  • Tests using fail_or_die leave arguments on the stack.
  • Some tests start with the comment "% to be run before run_test.sli", but this file does not exist any more.
  • Not all tests run ResetKernel or explicitly leave the namespace of unittest, but some do.
  • The line "userdict /resolution known { 0 << /resolution resolution >> SetStatus } if" seems to be unnecessary in most tests.
@hakonsbm

This comment has been minimized.

Show comment
Hide comment
@hakonsbm

hakonsbm Jun 30, 2017

Contributor

@jhnnsnk Thank you for your review. I have fixed some of the inconsistencies, but others are not that strictly enforced in the testsuite. Addressing your points

  • The purpose of this PR is to make the existing topology tests available with installcheck. But I have created an issue, #772, on creating new tests to cover the functionality mentioned.
  • While it is good practice to not leave arguments on the stack, this is not strictly enforced in many tests in the testsuite.
  • I have added ResetKernel to tests lacking this. Leaving namespace may also be good practice, but is not strictly enforced in the testsuite.
  • The change of resolution is removed from tests that do not need it.
Contributor

hakonsbm commented Jun 30, 2017

@jhnnsnk Thank you for your review. I have fixed some of the inconsistencies, but others are not that strictly enforced in the testsuite. Addressing your points

  • The purpose of this PR is to make the existing topology tests available with installcheck. But I have created an issue, #772, on creating new tests to cover the functionality mentioned.
  • While it is good practice to not leave arguments on the stack, this is not strictly enforced in many tests in the testsuite.
  • I have added ResetKernel to tests lacking this. Leaving namespace may also be good practice, but is not strictly enforced in the testsuite.
  • The change of resolution is removed from tests that do not need it.
@jhnnsnk

This comment has been minimized.

Show comment
Hide comment
@jhnnsnk

jhnnsnk Jun 30, 2017

Contributor

Thanks for addressing my points.
👍

Contributor

jhnnsnk commented Jun 30, 2017

Thanks for addressing my points.
👍

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 30, 2017

Contributor

Release note: Added tests for topology module to testsuite.

Contributor

heplesser commented Jun 30, 2017

Release note: Added tests for topology module to testsuite.

@heplesser heplesser merged commit 1d285ca into nest:master Jun 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hakonsbm hakonsbm deleted the hakonsbm:topology_tests branch Jun 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment