Skip to content

[MRG] Refactor test_logger.py as per pytest design.#453

Merged
lesteve merged 1 commit intojoblib:masterfrom
kdexd:pytestify-test-logger
Dec 12, 2016
Merged

[MRG] Refactor test_logger.py as per pytest design.#453
lesteve merged 1 commit intojoblib:masterfrom
kdexd:pytestify-test-logger

Conversation

@kdexd
Copy link
Copy Markdown

@kdexd kdexd commented Dec 12, 2016

Third Phase PR on #411 (Succeeding PR #450)

This PR deals with refactor of tests in test_logger.py to adopt the pytest flavor.

There is one only test method, which used a temporary file to log output. The setup and teardown for the same, using tempfile module has been removed, and instead, pytest's own tmpdir fixture is specified as function parameter.

Instead of raising an AssertionError based on condition, direct usage of assert keyword is used.

@kdexd
Copy link
Copy Markdown
Author

kdexd commented Dec 12, 2016

@lesteve This was a small module ! Merging this PR would be easier and faster than previous ones I guess 😄

@kdexd kdexd force-pushed the pytestify-test-logger branch from d7712f5 to 07e8585 Compare December 12, 2016 14:50
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Dec 12, 2016

LGTM, merging thanks!

Thanks also for doing the pytest migration in chunks, this helps the review process a lot!

tmpdir is quite nice by the way, especially compared to our own setup/teardown code.

@lesteve lesteve merged commit 1e4232b into joblib:master Dec 12, 2016
@kdexd
Copy link
Copy Markdown
Author

kdexd commented Dec 12, 2016

Wohoo that was really quick ! Well I realize that I am not letting you rest, but I'm enjoying this, we are nearing completion 🎉

@kdexd kdexd deleted the pytestify-test-logger branch December 12, 2016 15:02
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Dec 12, 2016

Wohoo that was really quick ! Well I realize that I am not letting you rest, but I'm enjoying this, we are nearing completion 🎉

It's nice indeed when something is progressing that quickly so we want to keep the momentum going. Having said that your last PRs are clearly an outlier (on the low side) in terms of PR reviewing time. I can not guarantee that it will be like this for the rest of times, so enjoy it while it lasts ;-).

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.

2 participants