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] use tox for running tests locally #696

Merged
merged 12 commits into from
Dec 15, 2020
Merged

[MRG] use tox for running tests locally #696

merged 12 commits into from
Dec 15, 2020

Conversation

luizirber
Copy link
Member

@luizirber luizirber commented Jul 10, 2019

Benefits:

  • Only need to install tox, and it takes care of installing all other deps.
  • Isolate code and temp files, avoiding testing the code in the dir (instead of the installed code)

(This is a slim version of #680, skipping the fix_lint parts that change A LOT of code)

Checklist

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed without a major version increment. Changing file formats also requires a major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@luizirber luizirber force-pushed the build/tox_travis branch 2 times, most recently from c8160be to 015bf25 Compare July 10, 2019 03:58
@luizirber luizirber changed the title use tox for running tests use tox for running tests locally Nov 22, 2019
@luizirber luizirber added this to the post-3.0 milestone Dec 30, 2019
@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #696 (42ba7e5) into latest (12c7da8) will increase coverage by 10.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           latest     #696       +/-   ##
===========================================
+ Coverage   83.32%   93.95%   +10.63%     
===========================================
  Files         103       98        -5     
  Lines        9600    14615     +5015     
  Branches        0     1434     +1434     
===========================================
+ Hits         7999    13732     +5733     
+ Misses       1601      637      -964     
- Partials        0      246      +246     
Flag Coverage Δ
python 93.95% <100.00%> (?)
rusttests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sourmash/__init__.py 100.00% <ø> (ø)
src/sourmash/__main__.py 83.33% <ø> (ø)
src/sourmash/cli/__init__.py 95.65% <ø> (ø)
src/sourmash/cli/categorize.py 100.00% <ø> (ø)
src/sourmash/cli/compare.py 100.00% <ø> (ø)
src/sourmash/cli/compute.py 100.00% <ø> (ø)
src/sourmash/cli/gather.py 100.00% <ø> (ø)
src/sourmash/cli/import_csv.py 100.00% <ø> (ø)
src/sourmash/cli/index.py 100.00% <ø> (ø)
src/sourmash/cli/info.py 100.00% <ø> (ø)
... and 161 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12c7da8...42ba7e5. Read the comment docs.

@luizirber luizirber changed the base branch from master to latest August 5, 2020 00:29
@luizirber luizirber changed the title use tox for running tests locally [WIP] use tox for running tests locally Aug 5, 2020
@luizirber luizirber force-pushed the build/tox_travis branch 6 times, most recently from a47cd64 to 1eb8804 Compare August 12, 2020 21:47
@luizirber luizirber added the 4.0 issues to address for a 4.0 release label Nov 1, 2020
@luizirber
Copy link
Member Author

luizirber commented Nov 1, 2020

This branch runs black and some other of the pre-commit formatters, and shows how it will look like. I'm not adding to this PR because... it is massive =]

move most of setup.py to pyproject.toml and setup.cfg
use isolated build for package creation
isolate test running
add some pyscaffold-inspired changes
update precommit
isort config
tox.ini updates based on tox-dev/tox
fix benchmarks for CI, add pypy wheels
add nix.shell for env setup
move sourmash to src/sourmash
coverage fixes
@luizirber luizirber force-pushed the build/tox_travis branch 4 times, most recently from d8cd27d to 10ead56 Compare December 10, 2020 00:40
@ctb
Copy link
Contributor

ctb commented Dec 10, 2020

reviewing tox part of things

I started out by creating a new conda environment with python3.8 in it, and running tox; got:

ERROR:  py39: InterpreterNotFound: python3.9
ERROR:   py38: InvocationError for command /Users/t/dev/sourmash.tox/.tox/py38/bin/python -m pip install --exists-action w '/Users/t/dev/sourmash.tox/.tox/.tmp/package/1/sourmash-4.0.0a4.dev2+g12c7da8.zip[test,storage]' (exited with code 2)
ERROR:  py37: InterpreterNotFound: python3.7
ERROR:  docs: InterpreterNotFound: python3.7

(I think the py38 / InvocationError is just a network error, so that's ok; will try again later.)

I'm not a fan of having the default be that a user has to have python 3.7, 3.8, and 3.9 installed just to run basic tests, so I suggest making that clear; I'll revisit the docs with that in mind.

tox -e py38 fails in naive conda environment

did pip install tox and got this;
not sure if it is expected:

python -m pytest --cov=. --cov-report term-missing --cov-report=xml
/Users/t/dev/sourmash.tox/.tox/py38/bin/python: No module named pytest
make: *** [coverage] Error 1
ERROR: InvocationError for command /usr/bin/make coverage (exited with code 2)

tox -e dev doesn't work

this one is listed in the developer docs...?

% tox -e dev
ERROR: unknown environment 'dev'

@ctb
Copy link
Contributor

ctb commented Dec 10, 2020

as far as moving code under src/sourmash goes, thanks, I hate it, but that's just because I'm a conservative kinda guy and hate learning new things if I don't have to :). Let me see how much slower it makes running the tests - that's my only remaining concern.

@ctb
Copy link
Contributor

ctb commented Dec 10, 2020

(as far as breaking existing PRs... most of them are yours!)

@luizirber
Copy link
Member Author

reviewing tox part of things

ERROR:  py39: InterpreterNotFound: python3.9
ERROR:   py38: InvocationError for command /Users/t/dev/sourmash.tox/.tox/py38/bin/python -m pip install --exists-action w '/Users/t/dev/sourmash.tox/.tox/.tmp/package/1/sourmash-4.0.0a4.dev2+g12c7da8.zip[test,storage]' (exited with code 2)
ERROR:  py37: InterpreterNotFound: python3.7
ERROR:  docs: InterpreterNotFound: python3.7

(I think the py38 / InvocationError is just a network error, so that's ok; will try again later.)

I'm not a fan of having the default be that a user has to have python 3.7, 3.8, and 3.9 installed just to run basic tests, so I suggest making that clear; I'll revisit the docs with that in mind.

Hmm, what version of tox are you running? I set up tox.ini to skip missing versions, it shouldn't be erroring with py39 and py37...

(is this running on this branch? or latest? Because docs should always be using 3.8, and I see it trying to use 3.7 🤔)

tox -e py38 fails in naive conda environment

did pip install tox and got this;
not sure if it is expected:

python -m pytest --cov=. --cov-report term-missing --cov-report=xml
/Users/t/dev/sourmash.tox/.tox/py38/bin/python: No module named pytest
make: *** [coverage] Error 1
ERROR: InvocationError for command /usr/bin/make coverage (exited with code 2)

It makes sense that running pytest directly will fail, because it is not installed. But this is the command that was executed inside tox -e py38, right?

This worked for me:

conda create -n tox_test python=3.8
conda activate tox_test
python -m pip install tox
python -m tox -e py38

(I used python -m {pip,tox} because I had them installed somewhere else, and my environment was not picking the conda-installed ones)

tox -e dev doesn't work

this one is listed in the developer docs...?

% tox -e dev
ERROR: unknown environment 'dev'

Hmm, I'm increasingly thinking that you're not in the build/tox_travis branch, or it is an older revision... dev is the last item in tox.ini, can you check if it is there?

@ctb
Copy link
Contributor

ctb commented Dec 10, 2020 via email

@luizirber
Copy link
Member Author

as far as moving code under src/sourmash goes, thanks, I hate it, but that's just because I'm a conservative kinda guy and hate learning new things if I don't have to :). Let me see how much slower it makes running the tests - that's my only remaining concern.

I get that, and it is a big change, but there are good reasons to use the src/ layout, especially to be sure we are testing/measuring coverage for the right files...

(as far as breaking existing PRs... most of them are yours!)

Good, so it is up to me to fix them =]

@luizirber luizirber changed the title [WIP] use tox for running tests locally [MRG] use tox for running tests locally Dec 14, 2020
Copy link
Contributor

@ctb ctb left a comment

Choose a reason for hiding this comment

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

Looks good overall, nice work!

I think it would be good to update the docs to point at src/sourmash/ rather than sourmash/.

Also, is there any documentation for the nix stuff?

@luizirber
Copy link
Member Author

I think it would be good to update the docs to point at src/sourmash/ rather than sourmash/.

Yup, I'll do that now.

Also, is there any documentation for the nix stuff?

I'll add docs and CI for that, it's what I've been using for local dev in my laptop (setting up Rust, python and tox), but since I do most of my dev in the remote workstation it is not as well tested/organized yet.

@ctb
Copy link
Contributor

ctb commented Dec 15, 2020 via email

@luizirber luizirber merged commit ca201cf into latest Dec 15, 2020
@luizirber luizirber deleted the build/tox_travis branch December 15, 2020 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 issues to address for a 4.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants