Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.97k stars 887 forks source link

Switch to pytest #3578

Closed digitalresistor closed 4 years ago

digitalresistor commented 4 years ago

This branch switches Pyramid to using pytest instead of nosetest.

digitalresistor commented 4 years ago

we can leave coverage on by default but there's several reasons it was off and reserved to specific targets:

  1. it slows everything down immensely

It's an additional 10 seconds on my laptop... but okay.

  1. we tend to only care about coverage on specific versions and it's a waste to test them all

¯\(ツ)/¯ I strongly believe that if someone is adding changes and runs them on Py 3.7 locally they should see coverage reporting without necessarily having to resort to installing Py 3.8 just to get a coverage report and know if their new changes are adequately covered.

  1. we have never tested coverage on windows before, and this enables it

This IMHO is a good idea, and code paths that differ between Linux/Windows should probably be marked as such, there is syntax to allow easily switching coverage reporting to on/off depending on the code path/which OS it's running on.

This would also enable coverage on macOS...

I would want to know if a code path is being skipped on either of those two OS's because of tests deselecting themselves on that platform or a variety of other reasons.

  1. we've had cases where coverage didn't run properly on pypy in the past and wanted to keep it out of there

¯\(ツ)/¯ this works without issues.

  1. we're not doing anything right now to properly combine the coverage from every target into a total when running tox -e coverage, especially not across windows/mac/linux so for that final report we are still picking a specific target we want to check

For right now, yes. But let's say we have a hack for Python 3.6 and 3.7 that is not in >3.8, with coverage enabled across all we make a simple change to the CI builder to build another environment and coverage combine picks them up. Without needing to make additional changes to tox.ini.

if you still feel strongly despite all these past, and some current, issues then I guess it's fine but if it were me I'd leave it off by default

I feel strongly about getting this merged. I've disabled coverage reporting except on a special case for right now. I don't like it, I don't think it's a smart idea, and I think the extra 10 seconds it takes to run the test suite are nothing in the grand scheme of things, but it is what it is.

digitalresistor commented 4 years ago

I think windows uses Scripts for bin

That has been an issue throughout the history of this document: https://github.com/Pylons/pyramid/blob/master/HACKING.txt#L75

Also, I don't believe Windows has an export function, or by default has pip3 or other tools installed somewhere in $PATH.

merwok commented 4 years ago

Ah right :confused: I wanted to be optimistic and ask whether links to external documentation such as https://docs.python.org/3/using/windows.html#configuring-python and https://docs.python.org/3/library/venv.html#creating-virtual-environments wouldn’t be enough, but even so the commands are not identical (py launcher recommended for windows, and even on unix there is a recommendation to use python -m module to be sure that it’s the same environment as python). So maybe the inelegant environment variable is the most lightweight thing to use for these docs :slightly_frowning_face:

digitalresistor commented 4 years ago

There was discussion about running coverage across all versions of Python versus just one. Does this sentence need to be revised?

That sentence was from the original HACKING.txt just relocated, and is technically still true, previously that was Python 2.7 + 3.6, now it's just Python 3.8 that's used for coverage reporting, with the assumption that what is covered in 3.8 is also covered in 3.5, 3.6, 3.7