python-trio / async_generator

Making it easy to write async iterators in Python 3.5
Other
95 stars 23 forks source link

Testing #2

Closed agronholm closed 7 years ago

agronholm commented 7 years ago

Testing setup improvements and PEP 8 fixes.

codecov-io commented 7 years ago

Current coverage is 100% (diff: 100%)

Merging #2 into master will not change coverage

@@           master    #2   diff @@
===================================
  Files           5     3     -2   
  Lines         512   164   -348   
  Methods         0     0          
  Messages        0     0          
  Branches        0    17    +17   
===================================
- Hits          512   164   -348   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update d190466...d9dbbfe

njsmith commented 7 years ago

Thanks for this -- I went through and cherrypicked some changes. In particular the spurious import of warnings, and you made me look more closely at how travis and coverage were working and discovered some screwiness. Plus inspired me to actually test sdists instead of installing directly from the source tree :-).

For everything else here I think I'm going to pass, though -- I'm not a big fan of extras, the setup.cfg settings seem inappropriate to me, I have no idea what's wrong with from . import and the current whitespace seems fine to me, I dislike tox's weird thing where it goes to all this trouble to install the package but then doesn't test the installed package, I don't like auto-running flake8, and the tox-travis config in thie branch is AFAICT broken anyway (e.g. it's not running flake8 even though it's configured to). Let me know if I'm missing anything important though?

agronholm commented 7 years ago

I don't like the fact that I'm installing your entire test suite in site-packages when I do pip install async_generator. It has no business being installed there. That's why the vast majority of projects these days have a separate directory for tests.

agronholm commented 7 years ago

Ok, you don't want to autorun flake8, fine. But why'd say that it's broken? I have a similar setup in all my projects and it works fine there.

agronholm commented 7 years ago

Oh, I think I made a mistake writing the tox-travis config in tox.ini. Should be 3.5.0 not 3.5 etc.

njsmith commented 7 years ago

I don't like the fact that I'm installing your entire test suite in site-packages when I do pip install async_generator. It has no business being installed there.

I'm not sure what that has to do with this PR? But in any case, this is a place where opinions differ, and I guess there are different subcultures. The vast majority of projects that I work on/with ship their testsuite as part of the package, because it should be easy for end-users to check that the code works on their system, not just on mine. (Maybe this is more common in the scientific software world? It's actually very common among e.g. numpy's users that the first thing they do after upgrading is import numpy; numpy.test().)

In this case we're talking about a project whose wheel is 16 KiB, so I don't think it's a big deal either way.

Ok, you don't want to autorun flake8, fine. But why'd say that it's broken? I have a similar setup in all my projects and it works fine there.

By "broken" I just mean, if you look at the travis logs for this PR, flake8 did not actually get run :-).

I suspect this might have to do with the fact that in .travis.yml the project tests on "3.5.0" and "3.5.2" instead of "3.5"? I didn't try to debug it though.

agronholm commented 7 years ago

If someone wants to test your project, they can git clone it and run tox. Installing the tests in site-packages is absolutely pointless.

agronholm commented 7 years ago

But I'm not going to argue this any further. Do what you want.

njsmith commented 7 years ago

I'd appreciate it if you could tone down the emotional rhetoric a little ("no business", "vast majority", "absolutely pointless") -- it's just software, and there are advantages and disadvantages to both approaches.

FWIW, the point is that released-code-as-actually-installed-in-an-actual-environment does not always behave the same as "git clone and run tox". I mean, hopefully they will, but you know, "trust but verify". This is especially true for software like numpy that depends on flaky system-dependent C libraries like the BLAS, but the principle is true in general. (tox normally doesn't even test the installed code, so it won't catch blatant errors like leaving out a critical file from the install!)

agronholm commented 7 years ago

I'd appreciate it if you could tone down the emotional rhetoric a little ("no business", "vast majority", "absolutely pointless") -- it's just software, and there are advantages and disadvantages to both approaches.

Alright, although I would argue that the "vast majority" comes from personal experience. I don't have actual numbers but about 8-9 out of 10 projects I've looked at has tests outside their source trees. I could of course be wrong about this being the norm, but it's what I've seen, especially in newer projects.

FWIW, the point is that released-code-as-actually-installed-in-an-actual-environment does not always behave the same as "git clone and run tox". I mean, hopefully they will, but you know, "trust but verify". This is especially true for software like numpy that depends on flaky system-dependent C libraries like the BLAS, but the principle is true in general. (tox normally doesn't even test the installed code, so it won't catch blatant errors like leaving out a critical file from the install!)

I don't quite understand how installing a project or not would make any difference regarding system-dependent C libraries when testing it.

As for the tox thing, you have a point – tox should be made to test the installed code, but I'm unsure how to go about it. Tox by default creates an sdist and installs it in every virtualenv it creates. So it should be available for tests, but if the packages live directly in the project directory, they will be imported instead of the installed versions.