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

Backport blackening, pytest to 2.0 #1291

Closed
bitprophet opened this Issue Sep 17, 2018 · 8 comments

Comments

Projects
None yet
1 participant
@bitprophet
Member

bitprophet commented Sep 17, 2018

Been frustrated with the difficulty (or the perceived difficulty, hasn't happened a lot here specifically but was a huge problem for a while in Fabric, Invoke) of forward-porting changes through the 2.4.x era pytest and black overhauls. Trying to backport those to 2.0 onwards so we have a nice clean break at 2.0 (this is in part to support dropping 1.x support entirely, which has its own additional barrier to easy maintainership).

Specifically this means taking work for, or around, #1100 and an un-ticketed set of PyCon 2018 sprint changes around implementing black support, and cherry picking them back to 2.0. I am hoping this goes as smoothly as such a thing can and that I am mostly going to be removing feature-related test blocks for stuff added in 2.1-2.4 (eg Ed25519 support).

Mostly making this a ticket to help track stuff I needed to tweak or defer.

@bitprophet bitprophet added the Support label Sep 17, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 17, 2018

Member

Got here by basically going "ok where did we start the pytest work" and that seems to be at e8c8f81 so I'm now cherry picking all apparently relevant commits from there up through the work for #1100 (aka 0bf3fa4).

First minor stumbling block is that 385856d results in complaints like so:

» inv test
Traceback (most recent call last):
  File "test.py", line 49, in <module>
    from test_client import SSHClientTest  # XXX why shadow the above import?
  File "tests/test_client.py", line 40, in <module>
    from .util import _support
ImportError: attempted relative import with no known parent package

This does not appear under eg the 2.4 branch so I'm not sure what's up and am hoping it was a temporary weirdness from using modern import practices with the old test.py, and that pytest's method of loading files works better.

Member

bitprophet commented Sep 17, 2018

Got here by basically going "ok where did we start the pytest work" and that seems to be at e8c8f81 so I'm now cherry picking all apparently relevant commits from there up through the work for #1100 (aka 0bf3fa4).

First minor stumbling block is that 385856d results in complaints like so:

» inv test
Traceback (most recent call last):
  File "test.py", line 49, in <module>
    from test_client import SSHClientTest  # XXX why shadow the above import?
  File "tests/test_client.py", line 40, in <module>
    from .util import _support
ImportError: attempted relative import with no known parent package

This does not appear under eg the 2.4 branch so I'm not sure what's up and am hoping it was a temporary weirdness from using modern import practices with the old test.py, and that pytest's method of loading files works better.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 17, 2018

Member

Another bit I hope doesn't screw me over: 2.4 was when we dropped Python 2.6/3.3 support. Pytest retains support for those on its end at least thru our used version (3.2.5) so as long as I avoid polluting the cherry-picks with such things, I might be ok...

Member

bitprophet commented Sep 17, 2018

Another bit I hope doesn't screw me over: 2.4 was when we dropped Python 2.6/3.3 support. Pytest retains support for those on its end at least thru our used version (3.2.5) so as long as I avoid polluting the cherry-picks with such things, I might be ok...

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 17, 2018

Member

Less of a concern, but still valid, is that downstream processes like third party package maintainers which may run tests or similar, will find this a breaking change (if they're manually invoking test.py instead of using inv test which has been in for a long time).

I don't like this, but I don't have time to do some sort of "handles both!" backport process, and those maintainers will need to change sometime anyhow (esp if they are offering 2.4 and up). My own needs to backport stuff w/o worry unfortunately trump most other concerns.

Member

bitprophet commented Sep 17, 2018

Less of a concern, but still valid, is that downstream processes like third party package maintainers which may run tests or similar, will find this a breaking change (if they're manually invoking test.py instead of using inv test which has been in for a long time).

I don't like this, but I don't have time to do some sort of "handles both!" backport process, and those maintainers will need to change sometime anyhow (esp if they are offering 2.4 and up). My own needs to backport stuff w/o worry unfortunately trump most other concerns.

bitprophet added a commit that referenced this issue Sep 17, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 17, 2018

Member

Things look mostly stable so far, tho I haven't tried to merge-up to 2.1+ yet.

Travis found that one minor thing Chris added and I cherry-picked (pytest-xdist) doesn't handle Python 2.6, so I cut it for now. Need to remember to re-add it in 2.4+ again.

Member

bitprophet commented Sep 17, 2018

Things look mostly stable so far, tho I haven't tried to merge-up to 2.1+ yet.

Travis found that one minor thing Chris added and I cherry-picked (pytest-xdist) doesn't handle Python 2.6, so I cut it for now. Need to remember to re-add it in 2.4+ again.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 17, 2018

Member

Ah here's a rub, this build proves that I forgot Invoke itself dropped 2.6/3.3 support in 0.22.0.

Technically speaking, I could drop 2.6/3.3 support in the older release lines in the sense that 'we no longer support it', especially if I can drop it purely on the dev/test side and not the install/API side. But that feels kinda shitty. Can I work around this?

Insofar as we're not actually importing the code under test in our Invoke tasks...maybe? Just try using a different Python interpreter for Invoke itself? Would guess Travis doesn't make that possible tho; and it'd cause issues re: dev requirements and the local virtualenv too (tho I guess I could just blast out the dev-requirements into the global system python env...but...ugh. esp given we say sudo: false...EDIT: yea, can't globally pip install in this env without sudo, I tried.


So it seems like the choices are "reimplement the Invoke bits in regular shell in travis.yml" or "remove 2.6/3.3 from the test matrix even back to 2.0":

  • Reimplementing is always annoying but may be least-bad, if it's feasible, have to check
    • especially if we can just straight up nix anything but regular testing (since we don't truly care if e.g. the Sphinx sites can't build, or if flake8 works, etc).
  • Trimming the test matrix is nice and easy and jives with "modern" support, but does leave us open to breaking our contract re: 2.6/3.3 support.
  • If we're gonna not actually test those interpreters, maybe best to just be honest & explicitly drop support after all?
    • Counterpoint: better to not explicitly BREAK support (e.g. by changing {0} to {} and so on) though; not catching 2.7'isms is not great but it's definitely better than actively preventing use of the software under 2.6.
    • Besides, if we'd go that far, we may as well just say "ok we've stopped supporting any Paramiko <2.4". which isn't great until enough time has passed...
Member

bitprophet commented Sep 17, 2018

Ah here's a rub, this build proves that I forgot Invoke itself dropped 2.6/3.3 support in 0.22.0.

Technically speaking, I could drop 2.6/3.3 support in the older release lines in the sense that 'we no longer support it', especially if I can drop it purely on the dev/test side and not the install/API side. But that feels kinda shitty. Can I work around this?

Insofar as we're not actually importing the code under test in our Invoke tasks...maybe? Just try using a different Python interpreter for Invoke itself? Would guess Travis doesn't make that possible tho; and it'd cause issues re: dev requirements and the local virtualenv too (tho I guess I could just blast out the dev-requirements into the global system python env...but...ugh. esp given we say sudo: false...EDIT: yea, can't globally pip install in this env without sudo, I tried.


So it seems like the choices are "reimplement the Invoke bits in regular shell in travis.yml" or "remove 2.6/3.3 from the test matrix even back to 2.0":

  • Reimplementing is always annoying but may be least-bad, if it's feasible, have to check
    • especially if we can just straight up nix anything but regular testing (since we don't truly care if e.g. the Sphinx sites can't build, or if flake8 works, etc).
  • Trimming the test matrix is nice and easy and jives with "modern" support, but does leave us open to breaking our contract re: 2.6/3.3 support.
  • If we're gonna not actually test those interpreters, maybe best to just be honest & explicitly drop support after all?
    • Counterpoint: better to not explicitly BREAK support (e.g. by changing {0} to {} and so on) though; not catching 2.7'isms is not great but it's definitely better than actively preventing use of the software under 2.6.
    • Besides, if we'd go that far, we may as well just say "ok we've stopped supporting any Paramiko <2.4". which isn't great until enough time has passed...
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 18, 2018

Member

After way too much idiocy around a typo, I have a dumb Travis level if/else working for that...next is some actual Python 2.7+'isms that are in the stuff I cherry-picked. Unrolling those and yet again need to make sure I re-unroll them at the merge w/ 2.4...

Member

bitprophet commented Sep 18, 2018

After way too much idiocy around a typo, I have a dumb Travis level if/else working for that...next is some actual Python 2.7+'isms that are in the stuff I cherry-picked. Unrolling those and yet again need to make sure I re-unroll them at the merge w/ 2.4...

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 18, 2018

Member

Real glad I remembered about git merge -s ours for the merge from 2.3 to 2.4...it was a huge nightmare of "I only ever want HEAD's copy of this but because the other parent technically changed these lines recently, have a ton of diffs". Still wasted a good 5-10 mins until then tho 😂

Member

bitprophet commented Sep 18, 2018

Real glad I remembered about git merge -s ours for the merge from 2.3 to 2.4...it was a huge nightmare of "I only ever want HEAD's copy of this but because the other parent technically changed these lines recently, have a ton of diffs". Still wasted a good 5-10 mins until then tho 😂

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 18, 2018

Member

Tests green on most branches by now...hooray! Only took basically all day 😑

Member

bitprophet commented Sep 18, 2018

Tests green on most branches by now...hooray! Only took basically all day 😑

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment