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

[MRG] Remove pytest warnings due to yield based tests. #461

Merged
merged 5 commits into from
Dec 14, 2016

Conversation

kdexd
Copy link

@kdexd kdexd commented Dec 14, 2016

Fourth Phase PR on #411

This PR targets on completely removing all the pytest warnings existing in all the tests at this stage. On master, at the base of this PR, there are 189 pytest warnings - mostly because pytest >= 3 has deprecated support of yield based tests.

  • Statements yielding a simple function (callable) have been removed and a normal function call is included there.

  • Statements yielding assert_* methods have been replaced with simple assert keywords as of now.

  • Finally --disable-pytest-warnings has been removed.

The goal of this PR is to eliminate all pytest warnings existing already.

@kdexd kdexd changed the title Replace assert* methods with assert keywords in test_memory.py. [WIP] Remove pytest warnings due to yield based tests. Dec 14, 2016
@kdexd
Copy link
Author

kdexd commented Dec 14, 2016

Oops, github assumed the commit message as PR title already !

@lesteve
Copy link
Member

lesteve commented Dec 14, 2016

Before I forget it would be good to remove assert_* that are not used. Good candidates are assert_true, assert_false, check if there are not others.

@kdexd kdexd force-pushed the yield-to-assert-simple-replace branch from e381ce0 to 8bc8899 Compare December 14, 2016 15:17
@kdexd kdexd force-pushed the yield-to-assert-simple-replace branch from 8bc8899 to 33275b1 Compare December 14, 2016 15:19
@kdexd
Copy link
Author

kdexd commented Dec 14, 2016

With test_memory.py and test_parallel.py done, we are already at 36 warnings already!

@kdexd
Copy link
Author

kdexd commented Dec 14, 2016

@lesteve ZERO WARNINGS ! 🏁 💚

@kdexd kdexd mentioned this pull request Dec 14, 2016
@kdexd kdexd changed the title [WIP] Remove pytest warnings due to yield based tests. [MRG] Remove pytest warnings due to yield based tests. Dec 14, 2016
yield check_same_results, dict(n_tasks=25, n_jobs=4, batch_size=7)
yield check_same_results, dict(n_tasks=10, n_jobs=4,
pre_dispatch="2*n_jobs")
check_same_results({'n_tasks': 2, 'n_jobs': 2, 'pre_dispatch': "all"})
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: typically you could have left the dict here. My personal preference is for the dict literal i.e. {'a': 1, 'b': 2}. Some people like dict apparently because it is more obvious to spot than {} and you don't have to type quotes around the keys. Basically: no strong motivation or no strong consensus -> leave it as it is.

Copy link
Author

Choose a reason for hiding this comment

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

My motivation is to keep things as uniform as possible, throughout the codebase. Maybe I will tune down its "intensity" 😁

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's a very fine equilibrium to find sometimes.

@lesteve
Copy link
Member

lesteve commented Dec 14, 2016

NIce very tidy ouput, I like this, great stuff, merging!

@lesteve lesteve merged commit 95d2b39 into joblib:master Dec 14, 2016
@kdexd kdexd deleted the yield-to-assert-simple-replace branch December 14, 2016 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants