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

Properly import missing functions in PyNEST hl_api #2213

Merged
merged 7 commits into from
Dec 6, 2021

Conversation

babsey
Copy link
Contributor

@babsey babsey commented Nov 22, 2021

This PR fixes the bug that load_help is missing in pynest/nest/lib/hl_api_info.py.

Updated: Also sli_func is missing in pynest/nest/lib/hl_api_helper.py.

These bugs exist since this PR: #2135

@babsey babsey changed the title Fix load_help Fix missing functions in hl_api Nov 22, 2021
@jougs jougs requested review from Helveg and jougs November 29, 2021 10:02
@jougs jougs added I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Bug Wrong statements in the code or documentation labels Nov 29, 2021
@jougs jougs added this to In progress in PyNEST via automation Nov 29, 2021
@jougs jougs added this to the NEST 3.2 milestone Nov 29, 2021
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

LGTM!

PyNEST automation moved this from In progress to Review Nov 29, 2021
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

Could you add some regression tests that cover the logic that use these functions? I implemented #2135 shotgun style with the unit tests, and these weren't covered 😛

@babsey
Copy link
Contributor Author

babsey commented Nov 30, 2021

It seems that #2135 produces more bugs than I registered.
Not everything can be tested, e.g. show_help_with_pager function.

However I added two functions test_load_help and test_help,
but without make html for the test, my added test functions might be obsolete.

if help_text:
self.assertTrue(isinstance(help_text, str))
else:
self.assetsIfNone(help_text)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this function? It looks autocorrect-suspiciously much like assertIsNone ;p

Copy link
Contributor

Choose a reason for hiding this comment

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

Unit tests are usually deterministic and based on a-priori knowledge, how come you need to check what help_text is? Shouldn't we find a case that is always str, and one that is always None so that we can confirm both cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

A general comment: Since we now use PyTest, please use plain asserts instead of self.assertXYZ.

@Helveg
Copy link
Contributor

Helveg commented Nov 30, 2021

It seems that #2135 produces more bugs than I registered. Not everything can be tested, e.g. show_help_with_pager function.

However I added two functions test_load_help and test_help, but without make html for the test, my added test functions might be obsolete.

What kind of bugs are they? The nest root module has undergone some cleanup and attribute magic has been added. Without tests, users and devs will probably have to bump into these bugs to find out what attributes got swept under the rug. That is unless we export all the attributes on the nest module before the changes, and copy paste them into a unit test that getattrs them?

@babsey
Copy link
Contributor Author

babsey commented Nov 30, 2021

The functionnest.help('ac_generator')' opens a pager to show documentation of the requested object and will return None.
I did not apply test for this case.
With an additional argument return_text=True, it should load text from the nest/share/doc/nest/html/models/ac_generator.rst.

In the testsuite html/rst files are not built (no make html), I conclude that the function nest.help('ac_generator', return_text=True)returns None.
But the help text exists in the deployment/production.

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

I tend to agree with @babsey that testing the help system in this way is not so easy, as it requires a full make doc to actually be of use. My suggestion is to remove the tests again, merge this PR and open an issue that could then be solved with a broader scope.

@@ -25,6 +25,11 @@

class TestHelperFunctions(unittest.TestCase):

def test_load_help(self):
help_text = nest.hl_api.load_help('ac_generator')
Copy link
Contributor

Choose a reason for hiding this comment

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

hl_api is gone and the load_help function is merely meant as a helper, so I'm not sure we actually should test it in this way.

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Of course, I meant to request changes instead of approving ;-)

@jougs jougs requested a review from Helveg December 2, 2021 08:38
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

Thanks for spotting these :)

@heplesser heplesser merged commit 3fb6598 into nest:master Dec 6, 2021
PyNEST automation moved this from Review to Done Dec 6, 2021
@babsey babsey deleted the fix-load-help branch December 17, 2021 14:24
@terhorstd terhorstd changed the title Fix missing functions in hl_api Properly import missing functions in pynest hl_api Jan 21, 2022
@terhorstd terhorstd changed the title Properly import missing functions in pynest hl_api Properly import missing functions in PyNEST hl_api Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
PyNEST
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants