Tests unnecessarily use pep8 internals

[Adam Williamson]

test_hacking_checks.py isn't aware of the pep8 library being renamed pycodestyle; it's trivial to have it work with either name.

The test also uses a rather unnecessarily baroque method to run only the K333 check; it's really not necessary to mock patch pep8's internals to do this, you can use its public API just as well.

Here's a patch which fixes both issues. I'm really not interested in jumping through fifteen thousand hoops and signing my firstborn over to the openstack foundation just to submit some patches I wrote in the course of distro package fixing. I work for Red Hat, which is (I believe) a corporate member of the the foundation, so I don't even know if it's appropriate/allowed for me to sign up as a personal member.

If it helps, I'm happy to declare this patch available under a very permissive license like CC-0, or just sign it over to a contributor, for the purpose of getting it merged.

[Steve Martinelli (stevemar)]

>> I'm really not interested in jumping through fifteen thousand hoops and signing my firstborn over to the openstack foundation just to submit some patches

I promise there are only 10-20 hoops and the firstborn is not necessary :)

I'll try and push this upstream on your behalf, thanks for the patch!

[Steve Martinelli (stevemar)]

Seems like we don't support pycodestyle in OpenStack yet [1]. Regardless, I can still use most of the code from the patch, I'll just remove the import handling.

I also added keystone as an affected project because we do the same check on our server side code as well. [2]

[1] https://review.openstack.org/#/c/325315/
[2] https://github.com/openstack/keystone/blob/master/keystone/tests/unit/test_hacking_checks.py

[Adam Williamson (awilliamson)]

Well, you could still include the pycodestyle handling, couldn't you? It doesn't stop the code working with pep8...if you'd rather prefer pep8 when both pycodestyle and pep8 are present, you can easily just invert the logic to ensure it.

I'll just note that the longer you delay moving openstack in general to pycodestyle because it doesn't behave precisely like pep8, the more painful it's going to be when you eventually do *have* to move it, but I don't really have much of an interest; I'm just trying to keep Fedora packages in line...:)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/416829

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/416830

[Steve Martinelli (stevemar)]

I spoke with the requirements team and theres a whole lot of background on why we're not using pycodestyle.

I narrowed down the scope of this bug to only address the issue of unnecessarily using pep8 internals. The requirements and hacking team will have to come up with a strategy for adoping pycodestyle that works for all openstack projects.

[Adam Williamson (awilliamson)]

Well, this change doesn't make anything *use* pycodestyle besides this test, and all this test is really doing is checking if the custom flake8 check that keystoneclient ships works. It just makes this specific test succeed if it happens to be the case that pycodestyle is present but pep8 is not, which can be the case in Fedora package builds because our flake8 is now built to use pycodestyle. So, I mean, I think you could make this specific change even if in general openstack wants to keep using its increasingly outdated pep8 and flake8 versions...this test itself is perfectly agnostic between pycodestyle and pep8, it can happily work with either just so long as it tries, so I don't see why it's a problem to let it do so.

Look at it as a small downpayment on the pycodestyle migration. It's gonna have to happen at some point, and you can fix this test for it right now, without breaking anything else! Gotta start somewhere. ;)

[Adam Williamson (awilliamson)]

So now I had cause to look even deeper into pycodestyle internals, I notice the first version of this patch isn't quite correct and was only working by luck. pycodestyle would register the check with *no* associated codes at all (because we didn't tell it any, and it can't find any from the function's docstring). Also, the value for `select` should be an iterable of strings, not a string.

By luck, we still got the right result. This is because checks with no associated codes *always* get selected, and the `check` value we passed got turned into ('K', '3', '3', '3'), and none of the codes for the built-in checks start with K or 3 (they all start with E or W) - so all built-in checks got ignored and our check got selected. But it really wasn't the way it was meant to work =)

This updated version actually works correctly; it registers the check with the 'K333' code associated, and then selects only checks associated with the 'K333' code.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/416829
Committed: https://git.openstack.org/cgit/openstack/python-keystoneclient/commit/?id=ffdab4ea43389b94f3c1d1ead5d97870d7c3c311
Submitter: Jenkins
Branch: master

commit ffdab4ea43389b94f3c1d1ead5d97870d7c3c311
Author: Steve Martinelli <email address hidden>
Date: Tue Jan 3 09:51:55 2017 -0500

    remove hacking checks from keystoneclient

    the only hacking check in keystoneclient is related to
    older versions of oslo libraries, which are no longer
    supported by keystoneclient, for example:

    $ pip freeze | grep oslo.utils
    oslo.utils==3.18.0
    $ python
    >>> import oslo.utils
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ImportError: No module named oslo.utils
    >>> import oslo_utils
    >>>

    Let's just remove the hacking check since theres no
    way someone could incorrectly import the older
    versions.

    Closes-Bug: 1652458

    Signed-off-by: Adam Williamson <email address hidden>

    Change-Id: I14165903b46d2fc26e8c9de591917893f58516db

[Adam Williamson (awilliamson)]

That works too!

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/416830
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=b63cc5f71dae6cecbdc91ff27bc469e5c3ea514e
Submitter: Jenkins
Branch: master

commit b63cc5f71dae6cecbdc91ff27bc469e5c3ea514e
Author: Steve Martinelli <email address hidden>
Date: Wed Jan 4 23:57:52 2017 -0500

    Use public interfaces of pep8 for hacking

    Rather than using mock and pep8 internals, we can use
    public interfaces of pep8 to register our custom
    hacking checks. Further, if pep8 isn't present for testing purposes,
    we can simply use pycodestyle. Many distributions are shipping the
    latter these days instead of pep8.

    Closes-Bug: 1652458

    Co-Authored-By: Ian Cordasco <email address hidden>
    Change-Id: Ica719703c54a295d10ea800467b25a5d0f65348a
    Signed-off-by: Adam Williamson <email address hidden>

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/python-keystoneclient 3.9.0 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone 11.0.0.0b3 development milestone.