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

Fix #762: PyNEST test result parsing confused by NEST output #788

Merged
merged 3 commits into from Aug 21, 2017

Conversation

@gtrensch
Contributor

gtrensch commented Jul 19, 2017

This PR installs the nosetests capturestderr plugin and corrcets some of the nosetests log messages. For more details please see issue #762.

@Silmathoron

The PR is straightforward enough, I just have a few questions, but I don't think there are any problems with the changes.

Show outdated Hide outdated .travis.yml
'''
def testClosenessNestLSODAR(self):
# Compare models to the LSODAR implementation.

This comment has been minimized.

@Silmathoron

Silmathoron Jul 26, 2017

Contributor

Why would you remove standard docstrings to replace them with comments? Does this have to do with the docstrings being printed somewhere during the test? I'm confused...

@Silmathoron

Silmathoron Jul 26, 2017

Contributor

Why would you remove standard docstrings to replace them with comments? Does this have to do with the docstrings being printed somewhere during the test? I'm confused...

This comment has been minimized.

@gtrensch

gtrensch Jul 26, 2017

Contributor

If present, nostests takes the first line of the docstring for reporting on the test cases. This led to something like this: The models should behave as iaf_cond_* if a == 0., b == 0. and ... ok
Most of the test cases make no use of a docstring. In that case the function name is reported. Hence I turned the docstring into line comments to make it clear and consistent.

@gtrensch

gtrensch Jul 26, 2017

Contributor

If present, nostests takes the first line of the docstring for reporting on the test cases. This led to something like this: The models should behave as iaf_cond_* if a == 0., b == 0. and ... ok
Most of the test cases make no use of a docstring. In that case the function name is reported. Hence I turned the docstring into line comments to make it clear and consistent.

This comment has been minimized.

@heplesser

heplesser Jul 31, 2017

Contributor

@gtrensch Tests should be commented using docstrings and the first line should give a concise description of the test. We haven't been particularly systematic about this, but changing docstrings to comments, is in my opinion the wrong way to go. In this particular case, the text is also fine as a docstring.

@heplesser

heplesser Jul 31, 2017

Contributor

@gtrensch Tests should be commented using docstrings and the first line should give a concise description of the test. We haven't been particularly systematic about this, but changing docstrings to comments, is in my opinion the wrong way to go. In this particular case, the text is also fine as a docstring.

This comment has been minimized.

@gtrensch

gtrensch Jul 31, 2017

Contributor

@heplesser using the docstring here was one of the confusions. As stated above, nosetests takes the first line, and only the first line of the docstring, to report on the test. I agree, well written docstrings are good coding practice. Just a few test cases make use of it. I'm not sure if it should be of the scope of this PR to do this refactoring task here.

@gtrensch

gtrensch Jul 31, 2017

Contributor

@heplesser using the docstring here was one of the confusions. As stated above, nosetests takes the first line, and only the first line of the docstring, to report on the test. I agree, well written docstrings are good coding practice. Just a few test cases make use of it. I'm not sure if it should be of the scope of this PR to do this refactoring task here.

@Silmathoron

Silmathoron approved these changes Jul 26, 2017 edited

Apart from the question regarding what we should do with capturestderr, I'm fine with this PR.
I think we should also discuss that in the context of #761 and check whether this is not already implemented in Nose2 (we are using pretty dated stuff, here)

'''
def testClosenessNestLSODAR(self):
# Compare models to the LSODAR implementation.

This comment has been minimized.

@heplesser

heplesser Jul 31, 2017

Contributor

@gtrensch Tests should be commented using docstrings and the first line should give a concise description of the test. We haven't been particularly systematic about this, but changing docstrings to comments, is in my opinion the wrong way to go. In this particular case, the text is also fine as a docstring.

@heplesser

heplesser Jul 31, 2017

Contributor

@gtrensch Tests should be commented using docstrings and the first line should give a concise description of the test. We haven't been particularly systematic about this, but changing docstrings to comments, is in my opinion the wrong way to go. In this particular case, the text is also fine as a docstring.

def testIafBehaviour(self):
# The models should behave as iaf_cond_* if a == 0., b == 0. and
# Delta_T == 0.

This comment has been minimized.

@heplesser

heplesser Jul 31, 2017

Contributor

This should remain a docstring, but with a better text, e.g.

"""
Ensure consistency with iaf_cond_* models.

The models shall behave as iaf_cond_* if a == b == Delta_T == 0.
"""
@heplesser

heplesser Jul 31, 2017

Contributor

This should remain a docstring, but with a better text, e.g.

"""
Ensure consistency with iaf_cond_* models.

The models shall behave as iaf_cond_* if a == b == Delta_T == 0.
"""

This comment has been minimized.

@gtrensch

gtrensch Jul 31, 2017

Contributor

@heplesser using the docstring here was one of the confusions. As stated above, nosetests takes the first line, and only the first line of the docstring, to report on the test. I agree, well written docstrings are good coding practice. Just a few test cases make use of it. I'm not sure if it should be of the scope of this PR to do this refactoring task here.

@gtrensch

gtrensch Jul 31, 2017

Contributor

@heplesser using the docstring here was one of the confusions. As stated above, nosetests takes the first line, and only the first line of the docstring, to report on the test. I agree, well written docstrings are good coding practice. Just a few test cases make use of it. I'm not sure if it should be of the scope of this PR to do this refactoring task here.

This comment has been minimized.

@heplesser

heplesser Jul 31, 2017

Contributor

I think I misunderstood. So even if nosetest only outputs a single line that still breaks our output parsing? I had though/hoped that our parsing would work as long as nose did no insert any line breaks. We should discuss how to proceed in the next VC.

@heplesser

heplesser Jul 31, 2017

Contributor

I think I misunderstood. So even if nosetest only outputs a single line that still breaks our output parsing? I had though/hoped that our parsing would work as long as nose did no insert any line breaks. We should discuss how to proceed in the next VC.

This comment has been minimized.

@gtrensch

gtrensch Aug 3, 2017

Contributor

@heplesser I might be wrong but I think the problem had nothing to do with line breaks contained in the docstring. Nest's outpout to stdout/stderr just simply comes in between, the report on the unit test printed by nosetests and the "... ok" string at the end of single test run. The line breaks came from the Nest output.
If the first line of the docstring would contain a meaningful test description, things are perfect. We have > 500 nose-based unit tests (I believe) where most of it do not make use of a docstring. There is some effort adding docstrings to all tests.
.. but yes, let's discuss this in the VC.

@gtrensch

gtrensch Aug 3, 2017

Contributor

@heplesser I might be wrong but I think the problem had nothing to do with line breaks contained in the docstring. Nest's outpout to stdout/stderr just simply comes in between, the report on the unit test printed by nosetests and the "... ok" string at the end of single test run. The line breaks came from the Nest output.
If the first line of the docstring would contain a meaningful test description, things are perfect. We have > 500 nose-based unit tests (I believe) where most of it do not make use of a docstring. There is some effort adding docstrings to all tests.
.. but yes, let's discuss this in the VC.

Show outdated Hide outdated .travis.yml
@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch Jul 31, 2017

Contributor

@heplesser indeed I'm also not happy with this approach. On the other hand we do similar things in the PIP Cython and Music install sections. I see this as an indermediate solution to fix the bug. If we move to nose2 or perhaps pytest this would be gone.

Contributor

gtrensch commented Jul 31, 2017

@heplesser indeed I'm also not happy with this approach. On the other hand we do similar things in the PIP Cython and Music install sections. I see this as an indermediate solution to fix the bug. If we move to nose2 or perhaps pytest this would be gone.

Show outdated Hide outdated .travis.yml
@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch Aug 3, 2017

Contributor

@lekshmideepu We can also download and install the tar file but isn't it the same source?

Contributor

gtrensch commented Aug 3, 2017

@lekshmideepu We can also download and install the tar file but isn't it the same source?

@Silmathoron

This comment has been minimized.

Show comment
Hide comment
@Silmathoron

Silmathoron Aug 3, 2017

Contributor

It think it is not: using pip to access Pypi means accessing a "repository of software for the Python programming language", which means that the package is supposed to be a lot more stable in time, whatever happens to the GitHub repo.

Contributor

Silmathoron commented Aug 3, 2017

It think it is not: using pip to access Pypi means accessing a "repository of software for the Python programming language", which means that the package is supposed to be a lot more stable in time, whatever happens to the GitHub repo.

@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch Aug 3, 2017

Contributor

@lekshmideepu @Silmathoron Thanks, I got the point. Installing from the Python package index means you can simply use "pip install". I didn't know this. This simplifies things a lot and as a nice side effect the plugin also appears in the list of the plugins installed.

Contributor

gtrensch commented Aug 3, 2017

@lekshmideepu @Silmathoron Thanks, I got the point. Installing from the Python package index means you can simply use "pip install". I didn't know this. This simplifies things a lot and as a nice side effect the plugin also appears in the list of the plugins installed.

@lekshmideepu

This comment has been minimized.

Show comment
Hide comment
@lekshmideepu

lekshmideepu Aug 4, 2017

Contributor

@gtrensch : Yes, @Silmathoron is right. Pypi packages are supposed to be stable when compared to installing a package from a GitHub repo.
You could see here the following:
Author: The SIO2 Project Team
Home Page: http://github.com/sio2project/nose-capturestderr

Contributor

lekshmideepu commented Aug 4, 2017

@gtrensch : Yes, @Silmathoron is right. Pypi packages are supposed to be stable when compared to installing a package from a GitHub repo.
You could see here the following:
Author: The SIO2 Project Team
Home Page: http://github.com/sio2project/nose-capturestderr

@lekshmideepu

This comment has been minimized.

Show comment
Hide comment
@lekshmideepu

lekshmideepu Aug 4, 2017

Contributor

@gtrensch : Plugin capturestderr was also shown in the earlier case when you were installing from the GitHub repo.

Contributor

lekshmideepu commented Aug 4, 2017

@gtrensch : Plugin capturestderr was also shown in the earlier case when you were installing from the GitHub repo.

@heplesser heplesser added this to the NEST 2.12.1 milestone Aug 21, 2017

@heplesser heplesser merged commit f2304c9 into nest:master Aug 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment