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

Throw exception when connecting fails #693

Merged
merged 5 commits into from Apr 7, 2017

Conversation

Projects
None yet
3 participants
@hakonsbm
Contributor

hakonsbm commented Mar 23, 2017

This PR fixes an issue where in some cases an attempt to create a connection fails silently, such as when trying to connect a multimeter to a generator. NEST will now throw an exception in these cases.

Additionally, a part of the regressiontest issue-211 that now throws an exception has been removed, and tests to make sure that NEST throws an exception to avoid silent failure has been added.

This fixes #578.

hakonsbm added some commits Dec 9, 2016

Fixed a test, added new tests
Removed a redundant test connecting device to device, and added a new
test for illegal connections.

@heplesser heplesser requested review from jakobj and heplesser Mar 24, 2017

@heplesser

This looks in principle sound to me, but @jakobj needs to review and comment on the change to the test for #211. There may have been a reason for silently skipping the device-device connection.

Travis currently fails due to formatting problems. I would suggest to wait until #691 is merged to master, merge that then from master into this branch.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 24, 2017

Contributor

@jakobj Since you wrote the test that is modified here, could you review the change in the issue-221.sli?

Contributor

heplesser commented Mar 24, 2017

@jakobj Since you wrote the test that is modified here, could you review the change in the issue-221.sli?

@jakobj

jakobj suggested changes Apr 4, 2017 edited

These are reasonably changes, however I think one return was missed and the tests need to be adapted. ps: sorry for taking so long to review.

@@ -68,11 +68,6 @@ n2 d4 << /weight 1. >> Connect
[g5 g5] [n1] Connect
g5 n3 << /weight 1. >> Connect
% device-device; but g5g5 should be ignored

This comment has been minimized.

@jakobj

jakobj Apr 4, 2017

Contributor

why did you remove this whole block? I think the connection g5 -> g4 is still valid, and actually created, so I would suggest just removing the first line ([g5 g5] [d4 g5] /one_to_one Connect) and keeping the others. This would imply that you need to remove one [5 4 0] from the target_conn list.

@jakobj

jakobj Apr 4, 2017

Contributor

why did you remove this whole block? I think the connection g5 -> g4 is still valid, and actually created, so I would suggest just removing the first line ([g5 g5] [d4 g5] /one_to_one Connect) and keeping the others. This would imply that you need to remove one [5 4 0] from the target_conn list.

@@ -177,3 +177,52 @@ ResetKernel
Last /target get 6 eq assert_or_die
} forall

This comment has been minimized.

@jakobj

jakobj Apr 4, 2017

Contributor

maybe it would make sense to get all devices from the modeldict and loop through all possible combinations instead of listing them manually here? this would avoid the need to add new tests here upon adding a new device and make sure that we do not forget a certain combination.

@jakobj

jakobj Apr 4, 2017

Contributor

maybe it would make sense to get all devices from the modeldict and loop through all possible combinations instead of listing them manually here? this would avoid the need to add new tests here upon adding a new device and make sure that we do not forget a certain combination.

@@ -413,7 +413,8 @@ nest::ConnectionManager::connect( index sgid,
// we do not allow to connect a device to a global receiver at the moment
if ( not source->has_proxies() )
{
return;
throw IllegalConnection( "The models " + target->get_name() + " and "

This comment has been minimized.

@jakobj

jakobj Apr 4, 2017

Contributor

should we throw a similar error in line546?

@jakobj

jakobj Apr 4, 2017

Contributor

should we throw a similar error in line546?

hakonsbm added some commits Apr 4, 2017

Expanded connection tests, added missing IllegalConnection.
Now tests creating of connection between all combinations of models,
checking for silent failures. Also, added a missing throw
IllegalConnection.
@hakonsbm

This comment has been minimized.

Show comment
Hide comment
@hakonsbm

hakonsbm Apr 4, 2017

Contributor

@jakobj Thank you for you review. Besides reinstating the missing test in issue-211 and adding a throw IllegalConnection in the connection manager, I have now expanded the test in test_connect.sli to check all combinations of models. The test fails if there is no thrown error, but no connection made.

Contributor

hakonsbm commented Apr 4, 2017

@jakobj Thank you for you review. Besides reinstating the missing test in issue-211 and adding a throw IllegalConnection in the connection manager, I have now expanded the test in test_connect.sli to check all combinations of models. The test fails if there is no thrown error, but no connection made.

@heplesser

Looks good except for one simplification in the test, see inline comment.

Show outdated Hide outdated testsuite/unittests/test_connect.sli
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 6, 2017

Contributor

@jakobj Could you have another look?

Contributor

heplesser commented Apr 6, 2017

@jakobj Could you have another look?

@jakobj

jakobj approved these changes Apr 7, 2017 edited

Looks good 👍 maybe it would be better to squash the commits before merging?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 7, 2017

Contributor

@jakobj I will merge as is---still a bit wary of re-writing history after it has become visible in public repositories.

Contributor

heplesser commented Apr 7, 2017

@jakobj I will merge as is---still a bit wary of re-writing history after it has become visible in public repositories.

@heplesser heplesser merged commit a8a0c3d into nest:master Apr 7, 2017

1 check passed

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

@hakonsbm hakonsbm deleted the hakonsbm:fix578_multimeter_connections branch Apr 19, 2017

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