Skip to content

[MRG + 1] Prevent nose from using docstring to name the tests in results #4432

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 23, 2015
Merged

[MRG + 1] Prevent nose from using docstring to name the tests in results #4432

merged 2 commits into from
Mar 23, 2015

Conversation

raghavrv
Copy link
Member

ISSUE

When docstrings are added to test_* functions, nose uses them as headers when a particular test fails. This is not very convenient, compared to the function name and the passed arguments (in case of generators).

And also as pointed out in the link shared by Joel, test generators do not respect the generators docstring... Most generators use naming similar to check_*. Docstrings need not be removed in such cases.

Whether the generators need to be decorated by a method which sets its description is debatable.

FIX

  • Convert all docstring to comments

@amueller @ogrisel Please take a look

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 1090810 on ragv:travis_ignore_docstring into 3b32099 on scikit-learn:master.

@raghavrv raghavrv changed the title [MRG + 1] Travis ignore docstring [MRG + 1] Prevent nose from using docstring to name the tests in results Mar 21, 2015
@raghavrv raghavrv force-pushed the travis_ignore_docstring branch from 1090810 to 4e633e6 Compare March 21, 2015 05:22
@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 4e633e6 on ragv:travis_ignore_docstring into 3b32099 on scikit-learn:master.

@raghavrv raghavrv force-pushed the travis_ignore_docstring branch from 4e633e6 to f763a34 Compare March 21, 2015 05:48
@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling f763a34 on ragv:travis_ignore_docstring into 3b32099 on scikit-learn:master.

@GaelVaroquaux
Copy link
Member

From a cursory look, this looks good to me.

It seems to me that you didn't add the "test_describe" decorated, as you suggested in the PR text. I actually prefer avoiding the complexity of the generator, so I prefer it this way, but I think that you should update the PR text.

@raghavrv
Copy link
Member Author

From a cursory look, this looks good to me.

Thanks :)

but I think that you should update the PR text.

done!

[...] I actually prefer avoiding the complexity of the generator [...]

I was also looking for other ways to display the generator name without having to use a decorator! Anyway I was planning the same for a subsequent PR, since this PR already has a huge diff...

@amueller
Copy link
Member

you can always recover a PR by checking out the branch of the PR and doing git reset --hard thebranchthatisnotffedupbeyondrepair

amueller
amueller reviewed Mar 23, 2015
View reviewed changes
sklearn/metrics/tests/test_classification.py
@@ -526,8 +520,6 @@ def test_classification_report_multiclass_with_unicode_label():
else:
report = classification_report(y_true, y_pred)
assert_equal(report, expected_report)


Copy link
Member

Choose a reason for hiding this comment

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

This looks like a mistake?

@ogrisel
Copy link
Member

ogrisel commented Mar 23, 2015

+1 as well.

ogrisel added a commit that referenced this pull request Mar 23, 2015
@ogrisel
Merge pull request #4432 from ragv/travis_ignore_docstring
8f67a81 
[MRG + 1] Prevent nose from using docstring to name the tests in results
@ogrisel ogrisel merged commit 8f67a81 into scikit-learn:master Mar 23, 2015
@amueller
Copy link
Member

thanks :)

@raghavrv
Copy link
Member Author

you can always recover a PR by checking out the branch of the PR and doing git reset --hard thebranchthatisnotffedupbeyondrepair

I did the exact same mistake I did a while ago in the PR that allows sparse y for grid_search... (pushed an empty branch to the PR's branch, which made git assume it got merged and hence closed it in a way it cannot be reopened...) 😭

I actually rebased it wrongly and hence checked out another branch b2 from master for cherry picking the commits from the dirty branch b1... wrongly assuming that the commits were cherry picked to b2 (where was not the case) I had pushed the empty branch b2 to that PR's upstream branch (upstream b1) which had closed the PR beyond recovery...

BTW Thanks for the review and merge :)

@amueller
Copy link
Member

I have no idea what github did with the new PRs that can not be reopened... hasn't happened to m, but doesn't sound like fun...

@raghavrv
Copy link
Member Author

Yea its definitely not fun :/ Especially since reopening a new PR means the discussion gets split up which is not quite convenient...

@raghavrv raghavrv deleted the travis_ignore_docstring branch March 23, 2015 15:31
This was referenced Mar 26, 2015
Closed
Merged
@raghavrv raghavrv mentioned this pull request Jan 24, 2016
Closed
@raghavrv raghavrv mentioned this pull request May 24, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@raghavrv @landscape-bot @GaelVaroquaux @amueller @ogrisel