Skip to content

[WIP] Refactor test_memory.py as per pytest design. #460

Closed
kdexd wants to merge 6 commits intojoblib:masterfrom
kdexd:pytestify-test-memory
Closed

[WIP] Refactor test_memory.py as per pytest design. #460
kdexd wants to merge 6 commits intojoblib:masterfrom
kdexd:pytestify-test-memory

Conversation

@kdexd
Copy link
Copy Markdown

@kdexd kdexd commented Dec 14, 2016

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

NOTE: Rebasing / force pushig didn't allow reopening of this PR, hence a new PR has been created as a replacement ( #466 )

@kdexd kdexd force-pushed the pytestify-test-memory branch from 46cb313 to 56c1fb5 Compare December 14, 2016 13:30
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Dec 14, 2016

I would rather have topic-based PRs rather than file-based PRs. It does help the review process a lot by focusing on one aspect at a time.

In this particular case having a PR that just removes all the yield from all the tests would be great. If that means replacing all the yield by just assert without trying to use parametrize I am more than fine with this.

Once this is done, remember to remove disable-warnings from setup.cfg.

@kdexd
Copy link
Copy Markdown
Author

kdexd commented Dec 14, 2016

In this particular case having a PR that just removes all the yield from all the tests would be great.

@lesteve This includes only tests of test_memory.py ? Because there are still 112 pytest warnings undone, and yield based tests of other files require parametrize. If yes then I'll search for possibilities in other files and include them in this PR, while shifting the assert_raises and tmpdir related commits in separate branch for a future PR maybe tomorrow...

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Dec 14, 2016

@lesteve This includes only tests of test_memory.py ?

When I say all the tests I meant all the tests from all the files.

yield based tests of other files require parametrize.

Do you have an example? Technically I do not think they require parametrize. Using parametrize is just a nicer way of writing the tests, although this is not always the case IMHO. What I was proposing is to just do a straightforward replacement of yield by assert. Using parametrize can be done later.

All I am trying to say is that I would do it in this order:

  • remove yield from the tests across the whole codebase (+remove disable-warnings from setup.cfg)
  • fix Nested parallel warnings test failing from time to time in python 3 #413. It is not great when "temporarily" skipping a test lasts too long
  • Try to write the tests in a nicer way using pytest functionalities. I am fine going back to a file-based approach at this point if this is more convenient for you

@kdexd
Copy link
Copy Markdown
Author

kdexd commented Dec 14, 2016

I am keeping this PR on a hold for a while, not doing yield => assert in ALL files here itself, I can change everything but not the name of branch.. When I deal with using parametrize in this file, I will use this PR and get it merged. Will reopen it in a while. Thanks for a clear roadmap 😄

@kdexd kdexd closed this Dec 14, 2016
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Dec 14, 2016

Thanks for trying to keep the github tracker tidy! Keeping the PR open would have been fine too, to be perfectly honest.

@kdexd
Copy link
Copy Markdown
Author

kdexd commented Dec 14, 2016

@lesteve yeah the tracker remains clean, plus I do this because if a third person comes here, he may get a clue that the PR was closed due to a change of plan and reopened later for a related task 😁

@kdexd
Copy link
Copy Markdown
Author

kdexd commented Dec 21, 2016

Rebasing / force pushing does not allow this PR to be reopened, I will make a separate one.

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