Reach 100% code coverage #13

jamadden · 2017-02-11T21:02:30Z

Mostly this means new tests. Sometimes its means "no cover" markers. In a few cases it means using a shorter, more declarative way to write the code (e.g., @total_ordering or ABCMeta) or unifying python2/python 3 branches as much as possible. Sometimes we got lucky and could drop code meant for 2.6 or earlier.

There are a few genuine bugfixes in here that were revealed by getting the coverage up.

I tried to limit the use of 'no cover' as much as possible. There are a few deeply nested functions that made it difficult to avoid, though.

This replaces #12.

Also enable pip caching on CI

…p pypy becasue it's much faster.

This helps with our coverage numbers. Under Python 2, we also start using the C implementation, and avoid some unnecessary encode/decode cycles, so we might be a tiny bit faster.

The str values of the exceptions.

Also when it raises an error, preserve the traceback if we're re-raising. This was invaluable in debugging this.

Had to use one 'no cover' sadly.

…entedError. This helps coverage numbers.

Notable: - float conversion now correctly handles bytes and unicode prohibited values on all versions. - remove a duplicate test function from test_logger. - remove test support in test_logger for < 2.7

- Fix StartupHandler on Python 3.

- SectionType has a few no-covers for exception cases that I didn't feel like mocking up all the conditions for. - Make the type objects, well, objects. They had a ``__metaclass__`` statement that I think did the same thing on Python 2.

There are unfortunately a few no-covers because of very deeply nested functions that are difficult to mock around. There is at least one genuine bug fix.

…ecause I couldn't come up with counter examples.

coveralls · 2017-02-11T21:05:02Z

Changes Unknown when pulling cc07327 on coverage-100 into ** on master**.

tseaver · 2017-02-12T13:24:20Z

@jamadden Now that it is at 100%, can you make coveralls not spew into the conversation itself? I hate getting emails from bots. :)

jamadden · 2017-02-12T13:29:07Z

I sure can and did. It's on by default, and the badge sure is pretty to look at, but it probably doesn't add much value. And there was that recent bug where it was commenting for every Travis build in the matrix which was really annoying; fortunately that seems to have been fixed.

mgedmin · 2017-02-13T13:37:29Z

Making the tests fail when coverage drops below 100% (as @tseaver likes to do, which I greatly admire) is a better way to maintain test coverage than those Coveralls comments that annoy everyone and that nobody ever reads.

jamadden · 2017-02-13T13:43:32Z

PRs are set to fail in that case.

mgedmin

LGTM with minor comments.

mgedmin · 2017-02-13T13:38:29Z

.coveragerc

+# things. Omit the main methods of test files.
+exclude_lines =
+    pragma: no cover
+    pragma no cover


This is very minor but it bugs me that we have two ways of spelling that pragma.

I guess there are no occurrences of 'pragma no cover' anymore so that can go away.

mgedmin · 2017-02-13T13:42:28Z

ZConfig/_compat.py

+
+    exec_("""def reraise(tp, value, tb=None):
+    raise tp, value, tb
+""")


I kind of hate the way this is split into multiple lines. I'd prefer

exec_(""" def reraise(tp, value, tb=None): raise tp, value, tb """)

or

exec_("def reraise(tp, value, tb=None): raise tp, value, tb")

For better or worse, that code is copied verbatim from six and various other zopefoundation packages that have done the same.

mgedmin · 2017-02-13T13:44:07Z

ZConfig/cfgparser.py

@@ -175,7 +170,8 @@ def replace(self, text):
            raise

    def error(self, message):
-        raise ZConfig.ConfigurationSyntaxError(message, self.url, self.lineno)
+        v = ZConfig.ConfigurationSyntaxError(message, self.url, self.lineno)
+        reraise(type(v), v, sys.exc_info()[2])


I don't understand the purpose of this change. Are you replacing the exception class and message but keeping the original traceback?

Perhaps define a helper function raise_with_same_tb(exc), because I see a lot of places in the diff that do this sort of thing with new temporary variables (all named differently) and type(...).

Yes, this is to keep the original traceback. There are lots of places in the code that do try:...except: self.error(). Debugging those was a nightmare without keeping the original traceback.

The simplest textual change was to change the (relatively few) error functions instead of changing all the callers.

mgedmin · 2017-02-13T13:54:33Z

ZConfig/datatypes.py

    if is_str:
-        if v.lower() in ["inf", "-inf", "nan"]:
+        if v.lower() in (u"inf", u"-inf", u"nan", b'inf', b'-inf', b'nan'):
            raise ValueError(repr(v) + " is not a portable float representation")


float() was fixed in Python 2.6 to interpret inf and nan in a portable fashion, so this check can be safely removed as ZConfig no longer needs to protect the users from this pitfall.

That would basically simplify this whole function into return float(v), probably making it unnecessary -- just use float() wherever float_conversion() is currently being called.

I'm not intimately familiar with the history there, but the linked docs still contain the phrase "When passing in a string, values for NaN and Infinity may be returned, depending on the underlying C library" (emphasis mine). It does say that float represents things a certain way, but it's not clear to me that it commits to parsing things a certain way.

I'd be happy to see this simplified, and that could arguably be a part of the changes for this PR. It's just not entirely clear to me its safe to do so.

The "depending on the underlying C library" bit remains from the earlier text, which warned that the specific inf/nan spellings did depend on the C library: http://svn.python.org/view/python/trunk/Doc/library/functions.rst?r1=59558&r2=59557&pathrev=59558. I wonder why it wasn't removed.

The "Float accepts the strings nan, inf and -inf for NaN and positive or negative infinity. The case and a leading + are ignored as well as a leading - is ignored for NaN." part is about the parsing. There are stdlib tests for this too.

The "Float always represents NaN and infinity as nan, inf or -inf." part is about the repr().

Ok, cool! Thanks for the pointers. Should this change be a part of this admittedly large PR, or should it wait? ( I am willing to create follow up issues for this and other things that come out of this discussion. )

Separate PR, I think, to keep things simple.

mgedmin · 2017-02-13T13:59:29Z

ZConfig/info.py

@@ -228,6 +220,8 @@ def __init__(self, name, sectiontype, minOccurs, maxOccurs, handler,
        # handler     - handler name called when value(s) must take effect,
        #               or None
        # attribute   - name of the attribute on the SectionValue object
+        assert maxOccurs is not None
+        # because we compare it to 1 which is a Py3 error


It is strange then that the constructor explicitly allows maxOccurs to be set to None, and that, e.g., def ismulti is comparing self.maxOccurs > 1 unconditionally.

Python 2.x returns False for None > 1. I cannot say whether that makes sense for ismulti or here.

BTW getdefault also unconditionally compares self.maxOccurs > 1.

I think the weirdness is partially because this is a subclass of an abstract class that allows subclasses to define the constructors differently (which they do, passing constants for some of the arguments). That comparison works on Py2, but fails on Py3.

Once we do the assert in the constructor, getdefault doesn't matter anymore?

Yeah, I think we should either fully support maxOccurs being None (maybe that is how you spell "unlimited"?) by adding code to make Python 3 handle this explicitly instead of failing, or we should require it to be not-None everywhere (i.e. remove the explicit "let it be None" case in the constructor and replace it with an assertion, or, better yet, an explicit ValueError).

I'm sorry that I don't currently have the time to investigate what maxOccurs is used for!

mgedmin · 2017-02-13T14:07:44Z

ZConfig/tests/test_config.py

+                                'malformed section start',
+                                self.loadtext, '<section')
+
+        # ConfigLoader.endSection raises this and its recaught and changed to a


Typo: its -> it's

mgedmin · 2017-02-13T14:08:27Z

ZConfig/tests/test_datatypes.py

@@ -93,14 +93,13 @@ def test_datatype_float(self):

        # These are not portable representations; make sure they are
        # disallowed everywhere for consistency.


As I mentioned above, this comment is no longer true since Python 2.6.

mgedmin · 2017-02-13T14:09:15Z

ZConfig/tests/test_datatypes.py

+        check(":80",                 AF_INET, (defhost, 80))
+
+        convert = self.types.get('socket-connection-address')
+        check(":80",                 AF_INET, ("127.0.0.1", 80))


That is a strange amount of blank spaces between function arguments. PEP-8 frowns on this.

Indeed. But that matches the way this is called in the rest of the function.

I can only imagine I was thinking that looking like a table would make this more readable. I wouldn't make the same mistake now. (I certainly hope.)

mgedmin · 2017-02-13T14:11:48Z

ZConfig/tests/test_info.py

+    suite.addTest(unittest.makeSuite(AbstractTypeTestCase))
+    suite.addTest(unittest.makeSuite(SectionTypeTestCase))
+    suite.addTest(unittest.makeSuite(SchemaTypeTestCase))
+    return suite


Strongly suggest

def test_suite() return unittest.defaultTestLoader.loadTestsFromName(__name__)

unless you have abstract classes inheriting from unittest.TestCase that would preclude this?

I haven't seen that idiom used anywhere in this codebase (or in any of the other zopefoundation projects I've touched so far). In fact, I wasn't actually aware of it.

I used this style because that's what was used in the rest of the code base. I'd be happy to see it change to a simpler style, but I submit that would belong in a separate PR that does the same for all the test modules.

I don't recall where I learned this trick, but I recall getting burnt a few times in Zopeland when I (or somebody else) wrote a new test class and then forgot the corresponding makeSuite() bit. Of course, measuring test coverage protects from that to some extent.

I also have doubts whether that refactoring belongs to this already-very-large PR (especially if we want to convert all the files in one go, rather opt for a more incremental approach and convert just those that are touched in this PR).

I have this habit where when I'm reviewing code I also tend to look for possible improvements in the surrounding codebase, and also for opportunities to share knowledge. But then I forget to state that that's what I'm doing, rather than asking for changes in this particular PR.

I was burnt a ridiculous number of times on this very PR by that issue :/ it was quickly noticed and fixed, but still.

I'm happy to create a follow up issue. And I will be doing the same for other projects where I've dealt with this (relstorage).

mgedmin · 2017-02-13T14:14:28Z

ZConfig/tests/test_schemaless.py

-    return doctest.DocFileSuite("schemaless.txt", package="ZConfig")
+    suite = unittest.makeSuite(TestSection)
+    suite.addTest(doctest.DocFileSuite("schemaless.txt", package="ZConfig"))
+    return suite


If you switch to using defaultTestLoader everywhere, a nice way to mix it with doctests is

def test_suite(): return unittest.TestSuite([ unittest.defaultTestLoader.loadTestsFromName(__name__), doctest.DocFileSuite(...), ])

tseaver · 2017-02-13T14:25:40Z

ZConfig/_compat.py

+    # Python 2
+    from io import BytesIO as _NStringIO
+
+NStringIO = _NStringIO


Why the alias?

tseaver · 2017-02-13T14:25:55Z

ZConfig/_compat.py

+    # Python 3 support.
+    import urllib.request as urllib2
+
+urllib2 = urllib2


Why rebind the name?

tseaver · 2017-02-13T15:36:17Z

ZConfig/cmdline.py

@@ -155,7 +157,7 @@ def get_section_info(self, type, name):
            bk = self.basic_key(s, pos)
            if name and self._normalize_case(s) == name:
                L.append((optpath[1:], val, pos))
-            elif bk == type:
+            elif bk == type: # pragma: no cover


Ugh, the shadowing of the type built-in is really ugly: I spent more than a bit of time trying to work out how the code worked. I wonder if we could rename the argument type_?

There would be a lot or renames to do throughout the codebase. type (among others) is commonly shadowed; I think this code predates new-style classes and type as a builtin.

I'd be happy to see that done, but I submit that it belongs in another PR.

type has been a built-in for a very long time (since before I discovered Python at version 1.2). I've never been particularly averse to shadowing it, though.

FWIW when I was reviewing this, I wanted to ask why this comparison wasn't using if bk is type to compare with the type builtin. Then I thought a bit more and decided maybe type was a local variable shadowing the builtin.

(Also, given that my Vim syntax-highlights type in a different color from regular variables makes me want to use type_ instead of shadowing.)

@mgedmin that was exactly what I thought when reading that diff (it took me a couple of minutes to stumble on the shadowing).

tseaver · 2017-02-13T15:40:24Z

ZConfig/components/logger/loghandler.py

+SysLogHandler = SysLogHandler
+HTTPHandler = HTTPHandler
+SMTPHandler = SMTPHandler
+Win32EventLogHandler = Win32EventLogHandler


I don't understand what "export these" means, or what that has to do with re-binding the name.

On another note: couldn't handlers.py just import them from logging.handlers?

This applies to this comment and two of the previous ones:

linters like flake8 and pylint (I run/ran both during this process) complain (rightfully) about unused imports. The way to fix that without adding #noqa or what have you is to use the import, and the simplest way to do that is to rebind it to the same name. This declarative statement seems reasonably clear to me that this name is a public part of the module definition, as opposed to a side-effect of the import.

In this particular case, it seemed important to leave the imports as they are for backwards compatibility.

Let's just hope linters won't decide to start warning about "useless assignment X = X" in the future ;)

I find the reassignment useless, and would prefer a comment like #noqa API to make the intent clearer.

I don't think we need to support backward-compatibility for re-exports of stdlib modules.

tseaver · 2017-02-13T15:41:45Z

ZConfig/components/logger/tests/test_logger.py

-except ImportError:
-    # Python 3 support.
-    import io as StringIO
+from ZConfig._compat import NStringIO as StringIO


Why not leave the name NStringIO, since we have to munge all the callers anyway?

It seemed like a slightly smaller change, but I admit it wasn't fully thought through. There was some evolution here. It can still change if desired.

jamadden · 2017-02-14T14:13:58Z

I created new issues for the things that have come out in the comments. Is this mergeable?

mgedmin · 2017-02-14T14:33:56Z

You still have a green check from me :)

The only thing I still find kind of distasteful in the new changes is this:

Perhaps define a helper function raise_with_same_tb(exc), because I see a lot of places in the diff that do this sort of thing with new temporary exception variables (all named differently) and reraise(type(exc), exc, sys.exc_info()[2]).

jamadden · 2017-02-14T14:37:02Z

Ah, now I understand what you mean by that. That should be easy enough to change and reduce the size of the PR a bit. I'll do that.

mgedmin · 2017-02-14T15:00:56Z

Thank you, looks great!

Remove 'try/except NameError' around uses of __file__ See #13 (comment)

jamadden added 18 commits February 11, 2017 06:39

Enable code coverage metrics on CI

f7bcf4e

Also enable pip caching on CI

coverage doesn't run on python 3.2 so we need to bump pypy3. Also bum…

e3b8227

…p pypy becasue it's much faster.

Py2/3: Unify handling of StringIO

e2906ca

This helps with our coverage numbers. Under Python 2, we also start using the C implementation, and avoid some unnecessary encode/decode cycles, so we might be a tiny bit faster.

100% coverage for __init__.py

ab810f1

The str values of the exceptions.

100% coverage for cfgparser.py

55a4523

Also when it raises an error, preserve the traceback if we're re-raising. This was invaluable in debugging this.

100% coverage for cmdline.py

b794868

Had to use one 'no cover' sadly.

Use ABCMeta and @AbstractMethod to prevent manually raising NotImplem…

f26a695

…entedError. This helps coverage numbers.

Many small coverage fixes, mostly in tests.

d4025b7

Notable: - float conversion now correctly handles bytes and unicode prohibited values on all versions. - remove a duplicate test function from test_logger. - remove test support in test_logger for < 2.7

100% for handlers.py; needed one NO COVER

478b0eb

100% coverage for loghandler.py

c3037f2

100% coverage for loghandler.py

e461ad3

- Fix StartupHandler on Python 3.

100% coverage for datatypes.

e13cd13

100% coverage for info.py

1fc4db3

- SectionType has a few no-covers for exception cases that I didn't feel like mocking up all the conditions for. - Make the type objects, well, objects. They had a ``__metaclass__`` statement that I think did the same thing on Python 2.

100% coverage for matcher.py

d6096c2

There are unfortunately a few no-covers because of very deeply nested functions that are difficult to mock around. There is at least one genuine bug fix.

100% coverage for loader.py

0e06387

100% coverage for schema.py

f06f052

100% coverage for url.py. Unfortunately I had to resort to no cover b…

f6b9b29

…ecause I couldn't come up with counter examples.

bugfix on python 3

cc07327

remove last remaining use of __metaclass__ and simplify reraising

f7b0f66

mgedmin approved these changes Feb 13, 2017

View reviewed changes

tseaver reviewed Feb 13, 2017

View reviewed changes

jamadden added 3 commits February 13, 2017 10:09

remove alternate spelling no longer used

e03fe5c

typos from reviewers

9efb07e

more consistently handle the aliases

eb210fc

jamadden mentioned this pull request Feb 14, 2017

Simplify test_suite construction #14

Closed

This was referenced Feb 14, 2017

Simplify maxOccurs handling #15

Closed

Simplify float conversion #16

Closed

Avoid shadowing builtins with variable names, like 'type' #17

Closed

Simplify most uses of reraise via a helper function

1dbc242

jamadden merged commit 010cb2b into master Feb 14, 2017

jamadden deleted the coverage-100 branch February 14, 2017 14:48

jamadden mentioned this pull request Feb 14, 2017

Enable code coverage metrics on CI #12

Closed

jamadden mentioned this pull request Feb 14, 2017

Remove 'try/except NameError' around uses of __file__ #20

Merged

jamadden added a commit that referenced this pull request Feb 14, 2017

Merge pull request #20 from zopefoundation/remove-file-tests

0d7ec6e

Remove 'try/except NameError' around uses of __file__ See #13 (comment)

		@@ -93,14 +93,13 @@ def test_datatype_float(self):

		# These are not portable representations; make sure they are
		# disallowed everywhere for consistency.

Reach 100% code coverage #13

Reach 100% code coverage #13

Conversation

jamadden commented Feb 11, 2017

coveralls commented Feb 11, 2017 • edited Loading

tseaver commented Feb 12, 2017

jamadden commented Feb 12, 2017

mgedmin commented Feb 13, 2017

jamadden commented Feb 13, 2017

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamadden Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamadden Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamadden commented Feb 14, 2017

mgedmin commented Feb 14, 2017

jamadden commented Feb 14, 2017

mgedmin commented Feb 14, 2017

coveralls commented Feb 11, 2017 •

edited

Loading

jamadden Feb 13, 2017 •

edited

Loading

mgedmin Feb 13, 2017 •

edited

Loading

jamadden Feb 13, 2017 •

edited

Loading