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

Tox env with no dependencies #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented May 21, 2020

Fixes #10

Two problems with this:

  • Merging reports does not seem to be working, coverage report shows a 13.9% decrease that is not actually accurate.
  • attrs is a dependency of pytest, so it gets installed. I tried to mock it but it's tricky, we need to prevent all imports from within itemadapter but not those within pytest; not sure it's worth it.

@elacuesta elacuesta force-pushed the tox-env-without-third-party-packages branch from bd07912 to f556354 Compare May 22, 2020 01:48
@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #22 into master will decrease coverage by 2.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #22      +/-   ##
===========================================
- Coverage   100.00%   97.95%   -2.05%     
===========================================
  Files            2        2              
  Lines           98       98              
===========================================
- Hits            98       96       -2     
- Misses           0        2       +2     
Flag Coverage Δ
#tests 97.95% <ø> (?)
Impacted Files Coverage Δ
itemadapter/utils.py 92.59% <0.00%> (-7.41%) ⬇️

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 aa2c8ae...37e109f. Read the comment docs.

@elacuesta elacuesta force-pushed the tox-env-without-third-party-packages branch from 70c77e5 to ad70a7d Compare May 22, 2020 15:10
@elacuesta
Copy link
Member Author

I tried several alternatives to combine coverage information, none of them seems to work as I expect 😓 #23, #24, #25, #26

@Gallaecio
Copy link
Member

Which parts of the coverage drop are unexpected?

@elacuesta
Copy link
Member Author

Here's the report for py38:

----------- coverage: platform linux, python 3.8.2-final-0 -----------
Name                      Stmts   Miss  Cover   Missing
-------------------------------------------------------
itemadapter/__init__.py       3      0   100%
itemadapter/adapter.py       66      0   100%
itemadapter/utils.py         27      6    78%   15-16, 26-27, 37-38
-------------------------------------------------------
TOTAL                        96      6    94%

This is the report for no-deps:

----------- coverage: platform linux, python 3.6.9-final-0 -----------
Name                      Stmts   Miss  Cover   Missing
-------------------------------------------------------
itemadapter/__init__.py       3      0   100%
itemadapter/adapter.py       66     11    83%   52, 62, 79, 81-86, 106, 108-110
itemadapter/utils.py         27     10    63%   17, 26-27, 39-46
-------------------------------------------------------
TOTAL                        96     21    78%

Those two ☝️ are independent runs, deleting the .coverage database in between them.

If the database is not deleted in between runs, the --cov-append pytest flag combines the coverage:

----------- coverage: platform linux, python 3.8.2-final-0 -----------
Name                      Stmts   Miss  Cover   Missing
-------------------------------------------------------
itemadapter/__init__.py       3      0   100%
itemadapter/adapter.py       66      0   100%
itemadapter/utils.py         27      2    93%   26-27
-------------------------------------------------------
TOTAL                        96      2    98%

Those two missing lines are expected, because attrs is installed by pytest itself.

@Gallaecio
Copy link
Member

Gallaecio commented Jun 2, 2020

py38 seems to have lower coverage when run in GitHub actions:

https://github.com/scrapy/itemadapter/pull/22/checks?check_run_id=730122799

----------- coverage: platform linux, python 3.8.3-final-0 -----------
Name                      Stmts   Miss  Cover   Missing
-------------------------------------------------------
itemadapter/__init__.py       3      0   100%
itemadapter/adapter.py       71      4    94%   62, 72, 89, 116
itemadapter/utils.py         27     11    59%   15-16, 26-27, 39-46
-------------------------------------------------------
TOTAL                       101     15    85%

On the bright side, this may mean that the upload to Codecov is not the issue here.

@elacuesta
Copy link
Member Author

elacuesta commented Jun 2, 2020

Interesting. Seems like by running tox -e py, the installed python version is used, but since "py" doesn't match any defined tox environment, the 3rd party dependencies are not installed, so scrapy is not available and thus some tests are skipped.
Only two lines are not covered at https://github.com/scrapy/itemadapter/actions/runs/122821432, so the report merging is indeed working. Thanks for making me notice 👍

@Gallaecio
Copy link
Member

Instead of using testenv as a place for minimum requirements for all tests, you could use it for the dependencies of the standard tests, hence you can remove the version-specific Tox configurations, and using py should not be a problem anymore.

It means some duplication between testenv and testenv:no-deps, but I think it may be worth it.

@Gallaecio
Copy link
Member

#33 shows a possible approach to work around pytest depending on attr.s.

It does increase the complexity of the tests, though.

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.

provide a test environment with missing dependencies
2 participants