Skip to content
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

Two false positives in roundup unittest TestCase #6

Open
jayvdb opened this issue Oct 18, 2015 · 6 comments
Open

Two false positives in roundup unittest TestCase #6

jayvdb opened this issue Oct 18, 2015 · 6 comments

Comments

@jayvdb
Copy link

jayvdb commented Oct 18, 2015

I've been testing this on a few very large repos, and finding the occasional error

In roundup (http://www.roundup-tracker.org/index.html), only the following:

test/test_templating.py:350:0: P103 other string does contain unindexed parameters
test/test_mailgw.py:2826:65: P103 other string does contain unindexed parameters
test/test_mailgw.py:2842:65: P103 other string does contain unindexed parameters
test/test_mailgw.py:2857:65: P103 other string does contain unindexed parameters

http://sourceforge.net/p/roundup/code/ci/default/tree/test/test_templating.py
http://sourceforge.net/p/roundup/code/ci/default/tree/test/test_mailgw.py

Definitely within the realms of reasonable use of noqa, but maybe worth fixing.

@xZise
Copy link
Owner

xZise commented Oct 18, 2015

The last three in test_mailgw have basically the same reason as #4. But the test_templating.py could be solved as that is not an assignment.

@jayvdb
Copy link
Author

jayvdb commented Oct 18, 2015

Detecting parent class unittest.TestCase could allow a separate code for violations in test cases, like violations in docstrings are a separate case. That would reduce the impact of #2 in pywikibot, as we could use the new code to allow unittests to use {} but prevent it in the main library.

That wont solve py.test based projects, but someone else might find devise a way to detect those, and they can always use file selection rules to prevent their tests being included.

All of the violations in https://github.com/GoogleCloudPlatform/gcloud-python (mentioned in googleapis/oauth2client#328 ) are in subclasses of unittest.TestCase, and I've noticed this is quite common in other projects.

@jayvdb jayvdb changed the title Two false positives in roundup Two false positives in roundup unittest TestCase Oct 18, 2015
@jayvdb
Copy link
Author

jayvdb commented Oct 19, 2015

A very faulty detection of pytest would be import pytest.

@xZise
Copy link
Owner

xZise commented Oct 19, 2015

And how would you want to get the parent class? It only parses one module, so it can only work if the class does only subclasses the TestCase class or classes from that module. If there should be a test specific code (what would happen to P102 and P101? Are they detected as normal and P103 only is changed) I think a more sensible solution is to specify a test path.

At least the test_templating issue is now solved (will upload a patch shortly).

@jayvdb
Copy link
Author

jayvdb commented Oct 19, 2015

Can you see whether import unittest appears in the __init__.py of the module being checked?

Using a test file glob might be the only sensible approach. Would yoube willing to add one as a configuration option for your plugin.
Otherwise people running flake8 need to run it with the different sets of options, and/or build two different flake8 rules in tox: one for tests, and one for other. Not a deal breaker, but would be nice.

IMO only P103 would be split into a separate code for 'test specific'.

@jayvdb
Copy link
Author

jayvdb commented Oct 26, 2015

With Astroid it is possible to get information about the class MRO.

>>> from astroid.test_utils import extract_node
>>> n = extract_node("""
... from unittest import TestCase
... class A(TestCase):
...    def test_foo(self):
...       pass
... """)
>>> print(n.mro())
[<ClassDef.A l.3 at 0x13ceb50>, <ClassDef.TestCase l.171 at 0x14430d0>, <ClassDef.object l.0 at 0xd6f290>]
>>> n.bases
[<Name.TestCase l.3 at 0x13cec10>]
>>> n.mro()[1].parent
<Module.unittest.case l.0 at 0x142b310>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants