100% coverage #30

jamadden · 2017-06-29T17:54:12Z

Fixes #29.

Also simplify the "minimal" environment command by simply skipping tests that can't be run.

tseaver

Overall, I don't like using raise NotImplementedError() as a replacement for pass: it clutters the intent.

tseaver · 2017-06-30T14:16:06Z

docs/testlayer.rst

@@ -76,24 +76,29 @@ ZCMLLayer

 We now want a layer that loads up some ZCML from a file. The default
 is ``ftesting.zcml``, but here we'll load a test ``testlayer.zcml``.
+We can also choose to provide extra ZCML features (here, "devmode").


This probably should either explain what devmode does, or provide a link to the docs which do.

Are there such docs about "devmode"?

Maybe just a link to docs about features in general (if there are such things).

There's documentation about features in general: http://zopeconfiguration.readthedocs.io/en/latest/narr.html#making-specific-directives-conditional

devmode is a feature that enables additional debugging features for developer convenience, like Django's DEBUG = True. E.g. in my Zope 3 projects I used hasFeature('devmode') to decide whether to expose Python tracebacks to the end user in my SystemError views.

That's what I wound up linking to:

We can also choose to provide extra ZCML features that are used to conditionally control processing of certain directives <http://zopeconfiguration.readthedocs.io/en/latest/narr.html#making-specific-directives-conditional>_
(here we use "devmode", a common condition for controlling development
options like debugging output).

tseaver · 2017-06-30T14:20:11Z

src/zope/component/standalonetests.py

+import sys # pragma: no cover (runs in subprocess)
+import pickle # pragma: no cover (runs in subprocess)
+
+def write(x): # pragma: no cover (runs in subprocess)


ISTM it would be better just to exclude the whole module via .configrc, e.g.:

[run] omit = src/zope/component/standalonetests.py

D'oh. I have no idea why I didn't do that.

tseaver · 2017-06-30T14:20:52Z

src/zope/component/testfiles/components.py

@@ -54,8 +54,7 @@ def __init__(self, context):
        self.context = context

 class Comp3(object):
-    def __init__(self, context):
-        self.context = context
+    pass


If the constructor is never exercised, then delete the whole class.

Done. Don't know why it's not showing up as completed.

tseaver · 2017-06-30T14:23:49Z

src/zope/component/tests/examples.py

@@ -54,7 +54,7 @@ class ISII(Interface):
    pass

 def noop(*args):
-    pass
+    pass # pragma: no cover


Delete it if never called.

Done. Don't know why it's not showing as completed.

tseaver · 2017-06-30T14:24:30Z

src/zope/component/tests/examples.py

@@ -123,8 +123,7 @@ def __init__(self, context):

 @implementer(I3)
 class Comp2(object):
-    def __init__(self, context):
-        self.context = context
+    pass


Delete the class if ctor is never called.

Done. Don't know why it's not showing as completed.

tseaver · 2017-06-30T14:32:37Z

src/zope/component/tests/test__api.py

@@ -1092,7 +1083,7 @@ def __conform__(self, iface):
            def queryUtility(self, iface, name, default):
                if iface is IFactory and name == 'test':
                    return _Factory()
-                return default
+                raise NotImplementedError()


pragma instead.

tseaver · 2017-06-30T14:35:37Z

src/zope/component/tests/test__declaration.py

@@ -80,22 +80,11 @@ def _run_generated_code(self, code, globs, locs,
                            fails_under_py3k=True,
                           ):
        import warnings
-        #from zope.component._compat import PYTHON3
-        PYTHON3 = False


This whole testcase class should be wrapped up in a skipIf(sys.version_info[0] > 2) check.

I don't think that's actually necessary. The fails_under_py3k argument can just be removed now...all the tests pass under both versions.

The "class advice" directives cannot be used at all under Python3.

That's true for the zope.interface versions---they specifically blacklist against that. But zope.component.adapts can be used in Python 3:

Python 3.5.3 (default, Apr 23 2017, 18:09:27) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> from zope.component._declaration import adapts >>> from zope.interface import Interface >>> class Foo(object): ... adapts(Interface) ... >>> from zope.component._declaration import adaptedBy >>> adaptedBy(Foo) (<InterfaceClass zope.interface.Interface>,)

Which is the same as under Python 2:

Python 2.7.13 (default, May 5 2017, 09:16:47) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> from zope.component._declaration import adapts >>> from zope.interface import Interface >>> class Foo(object): ... adapts(Interface) ... >>> from zope.component._declaration import adaptedBy >>> adaptedBy(Foo) (<InterfaceClass zope.interface.Interface>,)

So these tests all pass both places.

For my own reference, because I'd never looked into this before: The zope.interface versions are broken because they want to modify the __metaclass__ variable of the caller, which is what doesn't work on Python 3. They do this in order to arrange to be run after the class has been constructed and hence its bases determined, so that they can "flatten" all of the implements declarations from the parent classes into the child class, presumably for performance.

z.c.adapts doesn't have this issue, because it just simply assigns its argument to a value in the caller's locals. Parents aren't considered.

OK. I still think we should be deprecating adapts on the There should be one-- and preferably only one --obvious way to do it. principle.

tseaver · 2017-06-30T14:38:13Z

src/zope/component/tests/test_globalregistry.py

@@ -214,7 +219,7 @@ class Foo(object):
            pass
        @adapter(IFoo)
        def _handler(context):
-            assert 0, "DON'T GO HERE"
+            raise NotImplementedError()


ISTM that the assert is clearer (with a pragma:).

Assertions can be turned off. It's not common under test code (I hope) but it is possible.

tseaver · 2017-06-30T14:38:19Z

src/zope/component/tests/test_globalregistry.py

@@ -230,7 +235,7 @@ def test_w_adapts(self):
        class IFoo(Interface):
            pass
        def _handler(context):
-            assert 0, "DON'T GO HERE"
+            raise NotImplementedError()


tseaver · 2017-06-30T14:40:04Z

src/zope/component/tests/test_hookable.py

@@ -68,62 +68,38 @@ def bar():
    def test_cant_assign_original(self):
        from zope.component.hookable import hookable
        def foo():
-            pass
+            raise NotImplementedError()


Wouldn't we be better off just adding pass to the list of lines excluded by coverage.

In my opinion, no.

I'll try to explain my thinking about the use of pass vs NotImplementedError in tests.

pass is a legitimate thing to write with a specific meaning: This is a function, it should be called, although it doesn't actually do anything. If it's not called, coverage will go down, and something changed---there's probably a regression.

In contrast, raise NotImplementedError has a different but still specific meaning: this is a callable, and I have to give it a name for reasons out of my control, but it must not be called. If it is called, something changed---there's probably a regression.

I see those two cases as useful distinctions to make.

I guess the "must not be called" case could usually be spelled "self.fail", although one would have to capture the correct "self" (or the method "self.fail"). Or we could introduce a specific exception class ("MustNotBeCalledError") to raise instead. Or we could have a pre-defined function that we simply assign to the needed name ("foo") in most cases: foo = callable_that_is_never_called)...hey, that has the benefit of being a line shorter!

I wish there were a simple way to spell the pass (must be called) case that would result in test failures instead of just coverage decreases.

Thoughts?

@tseaver I updated the PR to use the foo = fails_if_called() approach instead of NotImplementedError. You're right, that does a better job capturing the intent and even reads easier, to me at least.

Random drive-by comment: to me raise AssertionError('should not be called') seems like the best way to spell must-not-be-called, and raise NotImplementedError('please override in subclasses') is the best way to spell abstract-method.

Add change note and badge to readme. Remove unused class and function. Omit standalonetests.py entirely from coverage. Another unused class. Incorporate feedback in test__api.py * Bring back _callFUT and make the queryAdapterInContext tests call it * Change raise NotImplentedError into specific fails_if_called() calls. Remove redundant argument now that all test in Test_adapts pass under all versions. Remove NotImplementedError from test_globalregistry.py Remove NotImplementedError from test_hookable.py Remove NotImplementedError from test_registry.py Remove NotImplementedError from test_security.py Remove NotImplementedError from test_zcml.py Remove NotImplementedError from test_factory.py Document ZCML feature and devmode. Really accept all arguments unless opted out.

jamadden · 2017-07-05T22:06:42Z

Thanks everyone!

jamadden force-pushed the issue29 branch from d1b8ac1 to 304af7a Compare June 30, 2017 11:56

tseaver requested changes Jun 30, 2017

View reviewed changes

jamadden force-pushed the issue29 branch from 9c4f1e9 to 712bb8e Compare June 30, 2017 16:01

tseaver approved these changes Jul 5, 2017

View reviewed changes

jamadden merged commit 8b7bfa5 into master Jul 5, 2017

jamadden deleted the issue29 branch July 5, 2017 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

100% coverage #30

100% coverage #30

jamadden commented Jun 29, 2017

tseaver left a comment

tseaver Jun 30, 2017

jamadden Jun 30, 2017

mgedmin Jul 5, 2017

jamadden Jul 5, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

tseaver Jun 30, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

jamadden Jun 30, 2017

tseaver Jul 5, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

tseaver Jun 30, 2017

tseaver Jun 30, 2017

jamadden Jun 30, 2017

jamadden Jun 30, 2017

mgedmin Jul 5, 2017

jamadden commented Jul 5, 2017

100% coverage #30

100% coverage #30

Conversation

jamadden commented Jun 29, 2017

tseaver left a 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

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 commented Jul 5, 2017