Compile tests of invalid options #39

loechel · 2017-02-05T18:10:47Z

No description provided.

stephan-hof

In general the pull request looks good to me.

Another thing where I'm not sure of is this dont_inherit flag on the various functions in compile.py.
If set to True the compile function inherits the future imports from the compile.py module. Right now there are no future imports in this file so even if set to True the flag has no effect at all.

I also think that inheriting the future imports from compile.py is rarely needed by the user of this functions. Since he has no control what is enabled or not, because it is hardcoded in compile.py (if there were any).
So I would vote for removing this flag from the public API at all.

stephan-hof · 2017-02-28T07:51:00Z

src/RestrictedPython/transformer.py

@@ -629,7 +629,8 @@ def visit_UnaryOp(self, node):
        """

        """
-        self.not_allowed(node)
+        return self.node_contents_visit(node)
+        # self.not_allowed(node)


Why this unnecessary comment out ? I thought this is what version control is for ?

should be fixed

stephan-hof · 2017-02-28T07:51:15Z

src/RestrictedPython/transformer.py

@@ -647,7 +648,8 @@ def visit_Not(self, node):
        """

        """
-        self.not_allowed(node)
+        return self.node_contents_visit(node)
+        # self.not_allowed(node)


Why this unnecessary comment out ? I thought this is what version control is for ?

stephan-hof · 2017-02-28T08:09:23Z

tests/test_compile.py

 from RestrictedPython import compile_restricted
 from RestrictedPython import CompileResult
 from RestrictedPython._compat import IS_PY2
+from tests import c_eval
+from tests import c_exec
+from tests import e_eval


Could you please explain why you moved from relative import to absolute import ?

To be honest, I don't like code in __init__.py at all.
What about moving all the code from __init__.py into a helpers.py and then use a relative import
from .helpers import c_eval.
I guess there are better names than `helpers.py', I used that one to express the idea.
What do you think ?

I do generally prefer absolute imports as this support readability and to understand where things come from.

The init.py is something we should avoid in total, it is directly derecommended in the pytest goog practice guide: http://doc.pytest.org/en/latest/goodpractices.html#choosing-a-test-layout-import-rules But without a init.py there is no namespace to import from.

regarding helpers I would also prefer that, but suggest utils.py or utilities.py.

@loechel @stephan-hof I put these functions into __init__.py because they are temporary. When we delete the old implementation we will delete them, too. (As I understand we will delete the old implementation before merging the Python3 branch to master.)

stephan-hof · 2017-02-28T08:10:50Z

tests/transformer/test_transformer.py

 from RestrictedPython import RestrictingNodeTransformer
 from RestrictedPython._compat import IS_PY2
 from RestrictedPython._compat import IS_PY3
 from RestrictedPython.Guards import guarded_iter_unpack_sequence
 from RestrictedPython.Guards import guarded_unpack_sequence
+from tests import c_exec
+from tests import e_eval
+from tests import e_exec


With the suggestion from above it would be from ..helpers import c_exec

I liked the relative import here as it makes clear that the code is in __init__.py which is not so clear to me when using tests. But as this is only temporary I do not mind.

stephan-hof · 2017-02-28T08:13:39Z

src/RestrictedPython/compile.py

+    #    mode='function',
+    #    flags=flags,
+    #    dont_inherit=dont_inherit,
+    #    policy=policy)


Could you please explain why you kept this artefact inside a comment ?

stephan-hof · 2017-02-28T08:16:54Z

setup.py

+
+      ],
+      setup_requires=[
+          'pytest-runner',


Could this be moved to test_requires. Why is pytest-runner needed to run setup.py ?

It just follows the documentation of pytest: http://doc.pytest.org/en/latest/goodpractices.html#integrating-with-setuptools-python-setup-py-test-pytest-runner

In some way, the extras_require test section is also redundant with test_requires

Ah thank you for the link. Looks like it is needed to make something like this working: python setup.py test.
I was asking, because for me it looks like that when RestrictedPython is installed also pytest-runner gets installed (correct me if I'm wrong). So when RestrictedPython is a dependency of another package (for example Zope2), then pytest-runner gets also installed, even if its only used for tests !
From my point of view stuff which gets installed outside of testing should be as minimal as possible.

A, ok, I think that is a common misunderstanding of setuptools and dependency declaration in Python Eggs.
http://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-dependencies
describe that only install_requires and extra_requires are used, if declared by external packages or pip install. Also only those are reflected in the *.egg-info/requires.txt file.
setup_requires is only evaluated if

python setup.py install

or

python setup.py develop

is used (--> see http://setuptools.readthedocs.io/en/latest/setuptools.html#new-and-changed-setup-keywords setup_requires).

I see, thanks for the heads up.
On the pypi page https://pypi.python.org/pypi/pytest-runner they have also a section called

Considerations Conditional Requirement

to avoid downloading if setup.py is called outside of tests. I'll play around with this at the weekend.

@loechel To my understanding buildout does something like python setup.py install so it will install pytest-runner in projects where RestrictedPython is a dependency. (I have seen this behaviour on other packages which used setup_requires.)

@stephan-hof https://pypi.python.org/pypi/pytest-runner#conditional-requirement looks promising but I did not yet use it anywhere.

loechel · 2017-02-28T12:58:52Z

@stephan-hof thanks for the review.
The flags and dont_inherit params are something I would like to keep, If I do understand https://docs.python.org/3.6/library/functions.html#compile correct, additional future imports could be added or the inheritance of calling this function is injected.

I could agree that that might be something that we don't want in context of RestrictedPython, but we do want to give the User a drop in Replacement of compile, so that

compile(<source>, filename="<inline>", mode="exec", ...)

and

from RestrictedPython import compile_restricted as compile
compile(<source>, filename="<inline>", mode="exec", ...)

works equivalent.

stephan-hof · 2017-02-28T15:42:40Z

I get your point, but then the functions in compile.py have to change.
Currently this inheritance is not compatible.
Let's assume the following code in module called test.py

from __future__ import print_function
from RestrictedPython import compile_restricted as compile
compile(<source>, filename="<inline>", mode="exec", ...)

Even if dont_inhert=False the future is not recognized in the compile_restricted function.
When compile_restricted is called it uses the futures from compile.py not from test.py
See:https://github.com/python/cpython/blob/2.7/Python/ceval.c#L4159

If this inheriting should be compatible with the original compile function something like this needs to be done.

parent_flags = sys._getframe(1).f_code.co_flags
parent_flags = parent_flags & PyCF_MASK
flags = flags | parent_flags

and then call compile with flags=flags and dont_inhert=True to avoid using future flags from compile.py.

Right now the users of RestrictedPython don't need this feature of future inheriting so I would leave it out. Typically the <source> comes from a different component anyway so inheriting the flags where restricted_compile is called could lead to surprising effects.

stephan-hof · 2017-03-01T07:18:57Z

@loechel I just realized that the discussion regarding the dont_inherit flag has nothing to do with the current pull request (sorry). I have seen this while looking through the changes so I brought it up.

I still think we should should have the discussion (other opinions are more than welcome), but this should not block merging this pull request.

loechel · 2017-03-02T09:03:37Z

@stephan-hof I did try to make a reminder on the documentation (93d634b) that we have to look at it.

could somebody ( @stephan-hof or @icemac ) approve this pull request so that it could be merged

stephan-hof

Looks good to me

icemac

Sorry I am a bit late with my review but I eventually made it through and have some questions.

icemac · 2017-03-09T07:04:00Z

.coveragerc

+omit =
+    src/RestrictedPython/tests
+    src/RestrictedPython/tests/*.py
+    tests/*.py


I think the tests should have 100 % coverage, too.

icemac · 2017-03-09T07:05:02Z

docs/conf.py

@@ -49,7 +49,7 @@

 # General information about the project.
 project = u'RestrictedPython'
-copyright = u'2016, Zope Foundation'
+copyright = u'2017, Zope Foundation and Contributors'


This should be '2016, 2017'.

icemac · 2017-03-09T07:13:24Z

setup.py

+
+      ],
+      setup_requires=[
+          'pytest-runner',


@loechel To my understanding buildout does something like python setup.py install so it will install pytest-runner in projects where RestrictedPython is a dependency. (I have seen this behaviour on other packages which used setup_requires.)

@stephan-hof https://pypi.python.org/pypi/pytest-runner#conditional-requirement looks promising but I did not yet use it anywhere.

icemac · 2017-03-09T07:15:50Z

setup.py

@@ -57,9 +64,6 @@ def read(*rnames):
          'release': [
              'zest.releaser',
          ],
-          'test': [
-              'pytest',
-          ],


This will break RestrictedPython [test�] when used in buildout. (Do we need it?)
If I remember correctly buildout is not able to handle test_requires. So maybe we should have both using the same list of packages.

icemac · 2017-03-09T07:19:21Z

tests/builtins/test_limits.py

+
+def test__limited_range_range_overflow():
+    with pytest.raises(ValueError):
+        _limited_range(0, 5000, 1)


Maybe this could be a custom exception derived from ValueError in future to tell the user a bit more about the actual error.

It would be nice here to assert the error text, too.

icemac · 2017-03-09T07:27:27Z

tests/transformer/test_transformer.py

 from RestrictedPython import RestrictingNodeTransformer
 from RestrictedPython._compat import IS_PY2
 from RestrictedPython._compat import IS_PY3
 from RestrictedPython.Guards import guarded_iter_unpack_sequence
 from RestrictedPython.Guards import guarded_unpack_sequence
+from tests import c_exec
+from tests import e_eval
+from tests import e_exec


I liked the relative import here as it makes clear that the code is in __init__.py which is not so clear to me when using tests. But as this is only temporary I do not mind.

icemac · 2017-03-09T07:29:06Z

tox.ini

@@ -19,14 +19,15 @@ extras =
    develop
    test
 commands =
-    py.test --cov=src --cov-report=xml {posargs}
+    py.test --cov=src --cov-report=xml --html=report-{envname}.html --self-contained-html {posargs}


Why do we need a n HTML report for each Python environment? What are you trying to achieve?

icemac · 2017-03-09T07:30:10Z

tox.ini

 setenv =
  COVERAGE_FILE=.coverage.{envname}
 deps =
    pytest
    pytest-cov
    pytest-remove-stale-bytecode
    pytest-mock
+    pytest-html


What are you trying to achieve with this new dependency?

icemac · 2017-03-09T07:46:10Z

src/RestrictedPython/transformer.py

        """
-        self.not_allowed(node)
+        return self.node_contents_visit(node)


It was only forbidden because there was no test for this method.
Did you write a test for it?

icemac · 2017-03-09T07:46:17Z

src/RestrictedPython/transformer.py

        """
-        self.not_allowed(node)
+        return self.node_contents_visit(node)


loechel added 2 commits February 5, 2017 19:08

check more invalid inputs

e2f7b0a

allow Not Statement in RestrictedPython, needed in AccesControll

63ea00b

loechel requested a review from icemac February 5, 2017 18:10

loechel added 8 commits February 6, 2017 10:55

add tests for Limits

ddda664

tests in testUtilities btest have already been moved to test_utilities

980e603

restructuring tests folder layout and imports for pytest

f93693c

correct params for compile_restricted_function according to old API

7736ab1

consolidate configs

2877cc0

add .eggs/ to git ignore list

e048d09

correct Copyright sentance

e664e1d

pytest-html included

c99a908

stephan-hof reviewed Feb 28, 2017

View reviewed changes

changed to match review and enhancement requests

0ba212f

stephan-hof approved these changes Mar 2, 2017

View reviewed changes

loechel merged commit 70eca19 into Python3_update Mar 3, 2017

loechel deleted the compile_tests branch March 3, 2017 14:18

icemac reviewed Mar 9, 2017

View reviewed changes

loechel mentioned this pull request May 18, 2018

handling of compile_restricted optional params flags and dont_inherit #114

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile tests of invalid options #39

Compile tests of invalid options #39

loechel commented Feb 5, 2017

stephan-hof left a comment

stephan-hof Feb 28, 2017

loechel Feb 28, 2017

stephan-hof Feb 28, 2017

stephan-hof Feb 28, 2017

loechel Feb 28, 2017

icemac Mar 9, 2017

stephan-hof Feb 28, 2017

icemac Mar 9, 2017

stephan-hof Feb 28, 2017

stephan-hof Feb 28, 2017

loechel Feb 28, 2017

stephan-hof Feb 28, 2017

loechel Feb 28, 2017

stephan-hof Feb 28, 2017

icemac Mar 9, 2017

loechel commented Feb 28, 2017

stephan-hof commented Feb 28, 2017

stephan-hof commented Mar 1, 2017

loechel commented Mar 2, 2017

stephan-hof left a comment

icemac left a comment

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

icemac Mar 9, 2017

Compile tests of invalid options #39

Compile tests of invalid options #39

Conversation

loechel commented Feb 5, 2017

stephan-hof 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

loechel commented Feb 28, 2017

stephan-hof commented Feb 28, 2017

stephan-hof commented Mar 1, 2017

loechel commented Mar 2, 2017

stephan-hof left a comment

Choose a reason for hiding this comment

icemac 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