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

Move tests_require to special "tests" extra for easier installation #1830

Merged
merged 1 commit into from Sep 22, 2021

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Sep 21, 2021

Based on the discussion in #1829, I thought it would be best to clear up some of the confusion in the developer's guide about running tests and test dependencies. I realized while commenting on that issue that tests_require in setup.py no longer serve any purpose as we don't use python setup.py test to run our tests. As far as I can tell there is no fully supported way currently to define your test dependencies except as an "extra" in setup.py.

As part of this work I looked into how you are supposed to define things in pyproject.toml. Although there are some examples, it didn't seem like a fully supported method. I mean, completely replacing our setup.py static information with metadata in pyproject.toml seems like more work and seems like it may not be fully supported by pip and other tools right now. I think we can save that for a later time and let larger projects run into the gotchas first. I know projects like xarray already define everything in their setup.cfg.

For reference: https://www.python.org/dev/peps/pep-0621

@djhoese djhoese added documentation cleanup Code cleanup but otherwise no change in functionality labels Sep 21, 2021
@djhoese djhoese self-assigned this Sep 21, 2021
@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #1830 (6f82157) into main (a6f26f3) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1830      +/-   ##
==========================================
- Coverage   93.03%   93.03%   -0.01%     
==========================================
  Files         268      268              
  Lines       39625    39628       +3     
==========================================
+ Hits        36865    36866       +1     
- Misses       2760     2762       +2     
Flag Coverage Δ
behaviourtests 4.72% <50.00%> (+<0.01%) ⬆️
unittests 93.60% <ø> (ø)

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

Impacted Files Coverage Δ
satpy/__init__.py 87.50% <50.00%> (-12.50%) ⬇️

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 a6f26f3...6f82157. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 93.546% when pulling 6f82157 on djhoese:build-test-requires into a6f26f3 on pytroll:main.

@mraspaud
Copy link
Member

Ftr, I started using poetry in another repo, works great, also for tests.

@djhoese
Copy link
Member Author

djhoese commented Sep 22, 2021

  1. I'm not a fan of lock files so if we switched or added poetry configuration in the future I would prefer not to commit the lock file.
  2. I'd still probably do everything with conda even if we had poetry config added. I wouldn't be surprised if there is a poetry plugin or work on conda to install dependencies from a pyproject.toml.
  3. I like that poetry's documentation is all pyproject.toml. I think it used to be a separate file.
  4. Is it possible to do "extras" with poetry beyond the dev dependencies?
  5. I'm sure we could list both setuptools/pip deps and poetry deps in a single pyproject.toml file. That wouldn't be that bad. I wonder if TOML has aliases/shortcuts that would even get rid of the duplicate lists of deps.
  6. Does possibly wanting poetry in satpy mean that this PR is not ready for merge?

@mraspaud
Copy link
Member

  1. yes, the lock file should probably be in the .gitignore
  2. I don't see poetry as a hinder. When you do poetry publish it just creates a pip-compatible distribution and puts it on pypi, from which conda just takes over to create a conda package. So poetry is really just a way to manage the package when you develop it the way I see it.
  3. yes, it's cool
  4. yep https://python-poetry.org/docs/pyproject/#extras
  5. What would be the use case for having both? still be able to do pip install -e .? because I think that would be the only thing that need to change, along with poetry run pytest instead of pytest .
  6. I think the build-system would be different for poetry, but the rest is fine I guess

For reference, the pyproject.toml file of my local package (which is a satpy plugin):

[tool.poetry]
name = "satpy-ctth-fm-plugin"
version = "0.0.0"
description = ""
authors = ["Martin Raspaud <martin.raspaud@smhi.se>"]
include = ["etc/*/*.yaml"]

[tool.poetry.dependencies]
python = "^3.7"
satpy = "^0.29.0"

[tool.poetry.dev-dependencies]
pytest = "^5.2"

[build-system]
requires = ["poetry-core>=1.0.0", "poetry-dynamic-versioning"]
build-backend = "poetry.core.masonry.api"

[tool.poetry.plugins."satpy.composites"]
"ctth_fm" = "satpy_ctth_fm_plugin"

[tool.poetry-dynamic-versioning]
enable = true

@djhoese
Copy link
Member Author

djhoese commented Sep 22, 2021

  1. Sorry, I meant a fancy understanding by conda where you could go conda env create -n my_satpy_env --from-pyproject=pyproject.toml and it would create/install/update the dependencies by reading the pyproject configuration. Even if supporting pip, poetry, and conda meant three separate lists of dependencies at least they'd be in one place.

  2. Github is forcing my numbering so this is a placeholder...

  3. ...

  4. Supporting users of pip and users of poetry with as small of a burden on us (at least that's the hope). For the pytest call, can't we have two separate calls, one for installing test deps and one for running the tests? Then we can still use pytest directly. Not sure if there is a benefit to that, but still wanted to mention it. This goes back to the conda point where I'm not sure I like the idea of depending on PyPI packages for all dependencies since we have so many complex compiled dependencies.

  5. I think switching to poetry is a bigger PR and this PR is a relatively simple change. That's why I asked.

@mraspaud
Copy link
Member

  1. gotcha, would be nice indeed
  2. for pytest, I was refering to the ci build actually, sorry. Indeed, pytest should still work in the satpy directory.
  3. I don't see anything big here preventing us from switching in the future.

@djhoese djhoese merged commit 1287817 into pytroll:main Sep 22, 2021
@djhoese djhoese deleted the build-test-requires branch September 22, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanup but otherwise no change in functionality documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pytest satpy/tests does not work
3 participants