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.

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

Sprint: Pytest conversion #864

Closed
31 of 37 tasks
effigies opened this issue Jan 25, 2020 · 38 comments · Fixed by #865
Closed
31 of 37 tasks

Sprint: Pytest conversion #864

effigies opened this issue Jan 25, 2020 · 38 comments · Fixed by #865
Labels
good first issue
Milestone
3.1.0

Comments

@effigies
Copy link
Member

effigies commented Jan 25, 2020 •
edited
Loading

Tasks

Some principles:

  • Try to avoid numpy test methods and decorators when a vanilla unittest/pytest construct exists. NOTABLE EXCEPTIONS: Tools for comparing arrays and floating points, such as assert_almost_equal.
  • Clear is better than brief.
  • ...

Original post follows:

Summary

nose is finally going to start failing on Python 3.9 due to some standard library API changes.

Python 3.9.0 is due to be released in October, but we might as well get out ahead of it. We don't need to rush, so I'm going to target the nibabel 3.1.0 release.

I put out a call on Twitter to see if anybody wanted to sprint on this, and got responses from @arokem, @chrisgorgo, @orduek, @robbisg and @anibalsolon (thanks to all for having the same handle on GitHub and Twitter).

This thread exists to coordinate the sprint.

Status

@djarecka and @chrisgorgo started on conversion in November:

There have been some changes in the tests since #840 was started, so it requires merging/rebasing.

Plans

To facilitate the transition, I've started the pytest branch, which is #840's commits, rebased on master.

The first thing to do is to identify all remaining tasks. Off the top of my head we need to:

  1. Eliminate the use of nose.tools.
  2. Move any nose test case classes to vanilla unittest features, which pytest should be able to handle transparently.
  3. Verify coverage doesn't change/increases.
  4. Run doctests in the documentation with pytest.

Logistics

Let's set at least one definite window that people want to be actively working on this and where I can commit to being more responsive to keep things from stalling out. We don't all need to align, but it would probably be best for me if there isn't a separate window for each participant.

Would it be useful to attempt to do this (semi-)synchronously, and use a chat room? Or would people rather go through the usual process of creating issues and pull requests?
@effigies effigies mentioned this issue Jan 25, 2020
Merged
@djarecka
Copy link
Collaborator

djarecka commented Jan 25, 2020 •
edited
Loading

I can work on this one of the day of the week Feb 1st-7th, preferably Mon-Wed. I believe I converted all tests from nibabel/tests, now we have to check each subdirectory of the nibabel and change the tests inside

@anibalsolon
Copy link
Collaborator

@effigies thank you for organizing this!

Tuesdays and Thursdays work for me in general. The chat room idea is nice for quick checks, but the issue/PRs process might be useful to keep tracking.

@robbisg
Copy link
Collaborator

robbisg commented Jan 25, 2020

In general I'm quite available during the week!!

@effigies
Copy link
Member Author

So would Tuesday, February 4 work for all three of you? Is there anybody that it wouldn't work for?

Also, does anybody have a Windows machine for debugging any issues there?

@orduek
Copy link
Collaborator

orduek commented Jan 26, 2020 via email

@anibalsolon
Copy link
Collaborator

Works for me, apart from the same two hours (unrelated tho)

I have a Windows at home, will setup the proper environment to test it.

@robbisg
Copy link
Collaborator

robbisg commented Jan 26, 2020

Works for me!! I am on Linux and avaliable considering GMT+2 ;)

@effigies
Copy link
Member Author

Okay. I'll be around 9am-5pm EST (UTC-5), except a couple of calls from 12:45pm-2pm EST. I'll see if I can push them around, but we should have plenty of synchronous time, even if I can't.

How would we like to do live chatting? I've gone ahead and set up a Riot.im channel (#nibabel:matrix.org). If people prefer, the Brainhack Mattermost is another option. I'd rather not set up gitter...

@anibalsolon
Copy link
Collaborator

Riot.im seems nice! I am fine with it.

@yarikoptic
Copy link
Member

FWIW we use riot and jitsi for DataLad. This I +1 on riot ;-)

@yarikoptic yarikoptic mentioned this issue Jan 27, 2020
Closed
@effigies effigies added this to the 3.1.0 milestone Jan 27, 2020
@effigies
Copy link
Member Author

Hi all, I just want to draw your attention to #865, which shows the diff of the pytest branch against master. You may want to have a look through to see what's already been done, and I've left comments about idioms we might want to try to work with, for consistency. Please feel free to comment with additional thoughts.

@djarecka
Copy link
Collaborator

djarecka commented Jan 30, 2020 •
edited
Loading

and I will prepare description of my "workflow" for changing the tests, so people can use it if they don't have better ideas.

@effigies
Copy link
Member Author

effigies commented Feb 4, 2020 •
edited
Loading

Hi all. I've updated the top post with some tasks. Some can likely be broken down further, but I figure I'll leave that to the person who starts working on a given one to decide to split up jobs.

Everybody in this thread thus far should have write access to the repository, so you can edit the top post to let people know you're working on it, break down the tasks, or link to a PR addressing it.

And as a reminder, we'll be chatting on https://matrix.to/#/!GMrAjYhYGQqbvuNvJX:matrix.org?via=matrix.org, and we can open a Jitsi chat if video call makes sense at any point.

I expect to be responsive from about 9am-12pm EST (UTC-5) and 1:30pm-5pm EST.

@djarecka
Copy link
Collaborator

djarecka commented Feb 4, 2020 •
edited
Loading

@effigies - sounds good! I will try to be at 9ish.

I completely forgot that I had still some tests in nibabel/tests that I didn't finish, and I put in those import pytest; pytestmark = pytest.mark.skip(), so I can run pytest on entire directory. I've already started working on them, so put my name next to them.

Also - I'm pretty sure that more tests should be edited...

@djarecka
Copy link
Collaborator

djarecka commented Feb 4, 2020 •
edited
Loading

These is my workflow for converting the tests:

  • using nose2pytest for the basic conversion:
    • the package is a bit abandon, as far as I remember I failed to install it with any newer python, so I use python=3.4 for this environment
    • running nose2pytest -v <test_directory>
  • finishing by hand, the most common changes needed to be done manually:
    • assert_raise -> pytest.raises
    • removing yield statements, it's often a good idea to use pytest.mark.parametrize instead
    • removing all nose imports
    • changing imports of nibabel.testing or ..testing to ..testing_pytest: this is a temporary file that should be renamed at the end to testing, but didn't want to remove the original one, so the tested that are not converted yet can still work.
      Note, that in most cases the functions that are imported from that module are not needed anymore, e.g. assert_true, assert_false, etc., so sometimes everything can be removed

@robbisg
Copy link
Collaborator

robbisg commented Feb 4, 2020

Ok! Thank you.

I am working on nibabel.streamlines.tests.test_array_sequence which seems easy to me for beginning!
I will check the tests on my laptop then I will push from my fork to be sure, I'm not messing up something!

@robbisg
Copy link
Collaborator

robbisg commented Feb 4, 2020

Now I will work on nibabel.streamlines.tests.test_tractogram!

@robbisg
Copy link
Collaborator

robbisg commented Feb 4, 2020

I will be out from 8am-10am (UTC-5) !

@djarecka
Copy link
Collaborator

djarecka commented Feb 4, 2020

@effigies - how can I find you on riot? I was able to login to my account, but not sure what to do next.
The link you sent yesterday doesn't go to the room for me

@effigies
Copy link
Member Author

effigies commented Feb 4, 2020

The room is #nibabel:matrix.org. I assumed the link would get people there.

@djarecka
Copy link
Collaborator

djarecka commented Feb 4, 2020

not really, but trying to find you...

@effigies
Copy link
Member Author

effigies commented Feb 4, 2020

If anybody needs help setting up a test environment, let me know and I can put together some instructions.

robbisg added a commit that referenced this issue Feb 4, 2020
@robbisg
TEST: pytest conversion #864 #865
01b1b7e
robbisg added a commit that referenced this issue Feb 4, 2020
@robbisg
TEST: test_tractogram to pytest #865 #864
ade7bde
@orduek
Copy link
Collaborator

orduek commented Feb 4, 2020

I'll be happy to get some help on that matter. thanks

@djarecka
Copy link
Collaborator

djarecka commented Feb 4, 2020

@orduek - hi, do you have conda installed on your laptop?

@orduek
Copy link
Collaborator

orduek commented Feb 4, 2020 via email

@djarecka
Copy link
Collaborator

djarecka commented Feb 4, 2020 •
edited
Loading

  • so you should start from forking the repository and cloning your fork to the laptop
  • than in the new python environment (probably 3.7 or 3.8 would be the best), you can install the nibabel package by running pip install -e .[all] in the nibabel directory. Try to import nibabel in python to see if it works
  • once it works you should change the branch from master to pytest and try to run some pytest tests, e.g.: pytest -vs nibabel/tests

let me know if this works and if you have any questions

@effigies
Copy link
Member Author

effigies commented Feb 4, 2020

Thanks, @djarecka.

Yeah, just to make very explicit, I use the following:

conda create -n nibabel-dev python=3.7
conda activate nibabel-dev
pip install -e ".[all]"

And then you can run nosetests or pytest, as needed. See:

nibabel/.travis.yml

Lines 136 to 196 in ab2e0b8

nosetests --with-doctest --with-coverage --cover-package nibabel nibabel \
-I test_api_validators \
-I test_arrayproxy \
-I test_arrayriters \
-I test_batteryrunners \
-I test_brikhead \
-I test_casting \
-I test_data \
-I test_deprecated \
-I test_deprecator \
-I test_dft \
-I test_ecat \
-I test_ecat_data \
-I test_endiancodes \
-I test_environment \
-I test_euler \
-I test_filebasedimages \
-I test_filehandles \
-I test_fileholders \
-I test_filename_parser \
-I test_files_interface \
-I test_fileslice \
-I test_fileutils \
-I test_floating \
-I test_funcs \
-I test_h5py_compat \
-I test_image_api \
-I test_image_load_save \
-I test_image_types \
-I test_imageclasses \
-I test_imageglobals \
-I test_keywordonly \
-I test_loadsave \
-I test_minc1 \
-I test_minc2 \
-I test_minc2_data \
-I test_mriutils \
-I test_nibabel_data \
-I test_nifti1 \
-I test_nifti2 \
-I test_openers \
-I test_optpkg \
-I test_orientations \
-I test_parrec \
-I test_parrec_data \
-I test_pkg_info \
-I test_processing \
-I test_proxy_api \
-I test_quaternions \
-I test_recoder \
-I test_remmovalschedule \
-I test_round_trip \
-I test_rstutils \
-I test_scaling \
-I test_wrapstruct
elif [ "${CHECK_TYPE}" == "test" ]; then
# Change into an innocuous directory and find tests from installation
mkdir for_testing
cd for_testing
cp ../.coveragerc .
pytest --cov nibabel -v --pyargs nibabel --deselect streamlines

@robbisg robbisg mentioned this issue Feb 4, 2020
Merged
@orduek
Copy link
Collaborator

orduek commented Feb 7, 2020

Hi @effigies, I can take on another task if needed.

@effigies
Copy link
Member Author

effigies commented Feb 7, 2020

If you've got the time, there are several submodules that haven't been claimed.

@robbisg
Copy link
Collaborator

robbisg commented Feb 10, 2020 •
edited
Loading

I'm working on nibabel.cifti2.tests and nibabel.cmdline.tests

Question:

 assert_raises(NotImplementedError, vi.__getitem__, (slice(None), 0))
 assert_raises(NotImplementedError, vi.__setitem__, (slice(None), 0), 0)
 assert_raises(NotImplementedError, vi.__delitem__, (slice(None), 0))

How to translate slice, I've done:

with pytest.raises(NotImplementedError):
        vi[(slice(None), 0)]
with pytest.raises(NotImplementedError):
        vi[(slice(None), 0)] = 0
with pytest.raises(NotImplementedError):
        vi.__delitem__((slice(None), 0))
assert_raises(*raiser)

Is there a more elegant way of this?

with pytest.raises(raiser[0]):
      raiser[1](*raiser[2:])

@effigies
Copy link
Member Author

effigies commented Feb 10, 2020 •
edited
Loading

Hi @robbisg,

I would translate these:

with pytest.raises(NotImplementedError):
    vi[:, 0]
with pytest.raises(NotImplementedError):
    vi[:, 0] = 0
with pytest.raises(NotImplementedError):
    del vi[:, 0]

And for assert_raises(*raiser), I would suggest simply changing to pytest.raises(*raiser). Deconstructing raiser for the sake of using with doesn't seem worth it to me.

@orduek
Copy link
Collaborator

orduek commented Feb 10, 2020

If you've got the time, there are several submodules that haven't been claimed.

I'll also take nibabel.cmdline.tests. Work on it tomorrow.

@robbisg
Copy link
Collaborator

robbisg commented Feb 10, 2020

Hey @orduek, I already did this module!

@effigies
Copy link
Member Author

I don't know that anything needs to be done in benchmarks, except improve the docstrings.

I think the next thing to do is to replace old nibabel.testing with nibabel.testing_pytest, update the imports everywhere and see if anything breaks.

@djarecka
Copy link
Collaborator

@effigies - anyone is working on nibabel.testing? I could work on this this week, but can't promise which day exactly.

@effigies
Copy link
Member Author

@djarecka I don't think so. Feel free.

@djarecka
Copy link
Collaborator

@effigies - I've made changes, but by mistake I push it directly to nipy, sorry for this, hopefully all test will pass (it did on my osx)...

@effigies
Copy link
Member Author

If somebody has time to review #891, I would appreciate it.

effigies pushed a commit to effigies/nibabel that referenced this issue Feb 20, 2020
@robbisg @effigies
TEST: pytest conversion nipy#864 nipy#865
4cdae21
effigies pushed a commit to effigies/nibabel that referenced this issue Feb 20, 2020
@robbisg @effigies
TEST: test_tractogram to pytest nipy#865 nipy#864
2dfc512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue
Projects
None yet
Milestone
3.1.0
Development

Successfully merging a pull request may close this issue.

TEST: Drop nose for pytest nipy/nibabel
6 participants
@yarikoptic @effigies @anibalsolon @robbisg @djarecka @orduek