itemadapter icon indicating copy to clipboard operation
itemadapter copied to clipboard

Tox env with no dependencies

Open elacuesta opened this issue 4 years ago • 8 comments

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 avatar May 21 '20 19:05 elacuesta

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%) :arrow_down:

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.

codecov[bot] avatar May 22 '20 01:05 codecov[bot]

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

elacuesta avatar May 22 '20 16:05 elacuesta

Which parts of the coverage drop are unexpected?

Gallaecio avatar May 22 '20 17:05 Gallaecio

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 :point_up: 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.

elacuesta avatar May 22 '20 17:05 elacuesta

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.

Gallaecio avatar Jun 02 '20 10:06 Gallaecio

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 :+1:

elacuesta avatar Jun 02 '20 17:06 elacuesta

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 avatar Jun 03 '20 05:06 Gallaecio

https://github.com/scrapy/itemadapter/pull/33 shows a possible approach to work around pytest depending on attr.s.

It does increase the complexity of the tests, though.

Gallaecio avatar Aug 13 '20 10:08 Gallaecio