include tests in installed package

[perrinjerome]

Fixes #231

This moves the tests to a sub-directory of RestrictedPython, so that they are installed along with the package.

This also required adjusting the imports from within the tests (making them relative imports after adding necessary __init__.py ) and making the tests pass the linter (they were not linted before, because they were not part of src I guess).
Some configuration that became unnecessary was also removed ( no need to include from tests in MANIFEST and no need to collect coverage from tests )

[d-maurer]

Jérome Perrin wrote at 2022-8-23 08:03 -0700:

Fixes #231

In my opinion, you should check the `git` history to find who has moved the tests; hopefully, the commit message also include the reason for the move.

[perrinjerome]

Jérome Perrin wrote at 2022-8-23 08:03 -0700:
Fixes #231
In my opinion, you should check the git history to find who has moved the tests; hopefully, the commit message also include the reason for the move.

I looked at history first, hoping to find one commit moving the tests, but they were not just moved, it was new tests created in tests by porting the tests from src/RestrictedPython/tests. That and it's what I tried to describe as "tests were gradually moved from src/RestrictedPython/tests to tests", but that was not really a clear explanation, let me try to explain with more details and example commits.

It started with 8f745a8 which introduced pytest and new tests in tests. At that time, the tests in src/RestrictedPython/tests were still all there.

During the development of version 4, tests were "ported" to new tests folder, like for example in these commits: ea7f250 6fcc399 c370a97 . Sometimes the commits adding tests in tests also removes the test from the old src/RestrictedPython/tests but sometimes not. 4e89925 was the last commit of this work, which removes all the old 3 implementation and all the old tests. After that, all the tests were in tests.

[perrinjerome]

@icemac you were very active in the development of version 4 and moving the tests to tests. Do you see a problem with moving the tests back to src/RestrictedPython/tests so that they get installed ? Thanks in advance

[icemac]

@perrinjerome I support moving the tests back to src. It was kind of a trick while porting to Python 3 to have the ability to tests both RestrictedPython implementations (the Py2 only and the universal one) next to each other but keeping the test code separate.

[icemac]

LGTM besides some comments. (The .meta.toml ones I could also fix after the merge.)

[icemac]

This file is generated, removing this line has to be changed in .meta.toml, too.

[icemac]

This file is generated, changing these lines has to done in .meta.toml, too.

[perrinjerome]

Thanks a lot @icemac ! I did not notice the .meta.toml, but I looked at https://github.com/zopefoundation/meta/tree/master/config and now I understand.

I applied the same changes in .meta.toml, so I guess this is good to merge

[d-maurer]

Jérome Perrin wrote at 2022-8-23 18:52 -0700:

I looked at history first, hoping to find one commit moving the tests, but they were not just moved, it was new tests created in `tests` by porting the tests from `src/RestrictedPython/tests`. ... It started with 8f745a8 which introduced pytest and new tests in `tests`. At that time, the tests in `src/RestrictedPython/tests` were still all there. During the development of version 4, tests were "ported" to new `tests` folder, like for example in these commits: ea7f250 6fcc399 c370a97 .

Most of those commits have been authored by @icemac. Maybe, he still knows why he moved to `pytest` and moved the tests outside the installation tree.

[loechel]

Well, it was my decission to move the tests out of the installed package scope.
From a security perspective tests should never be forced to be installed in a production environment. Tests are test, the should life outsite of the installed package scope. But I agree with @d-maurer that tests should always be distributed in source packages.

But as this ticket / pull request has already been merged, what should be done? I am in favoure of directly revert that and let the tests be in seperate scope. All what @perrinjerome wants is to run the the original tests after his monkey patch been applied. Thats the normal case and is directly supported by all test frameworks, so why install the test?

[loechel]

After even looking more into the pull-request. I veto on releasing any new RestrictedPython version with that merge included. It is a security issue and a absolute nogo. RestrictedPython was always planed at written so that it did not depend on any external package. If the tests lifes inside the package scope and have imports from external packages like pytest, which is even not declared in the setup.py, that is absolutly not accepatble.

@icemac please revert that merge.

[perrinjerome]

All what @perrinjerome wants is to run the the original tests after his monkey patch been applied. Thats the normal case and is directly supported by all test frameworks, so why install the test?

Maybe my use case was not clear, or maybe I misunderstood something.

The background story is that I have a package which depends on RestrictedPython and this packages use monkey patches to "extend" RestrictedPython. I want to confirm that the monkey patches do not break original behavior of RestrictedPython, so what I do in my package's test is: import my module applying monkey patches and then import RestrictedPython test suite and run it as part of my package test suite. I think this is only possible if RestrictedPython tests shipped with RestrictedPython package.

With RestrictedPython 3.6.0, I used something like this :

import mypackage.monkeypatches

def test_suite():
  suite = unittest.TestSuite()
  import RestrictedPython.tests.testCompile
  suite.addTest(RestrictedPython.tests.testCompile.test_suite())
  import RestrictedPython.tests.testUtiliities
  suite.addTest(RestrictedPython.tests.testUtiliities.test_suite())
  ...

When updating RestrictedPython this was no longer possible (but maybe I missed something ?). There is anyway another change that RestrictedPython switched to pytest, I am not sure it would be possible to use this same approach with pytest.

That was the context, but the main reason that made me suggest these changes was that other zopefoundation packages seem to all install the tests, as described in the issue ( #231 ) and it did not seem intentional that this package was an exception.

For sure, I don't have any problem with having these changes reverted, my use case might be a bit exotic use case.

[perrinjerome]

I have been looking a bit and I don't see the security issue though

[perrinjerome]

I have been looking a bit and I don't see the security issue though

I posted that before seeing #231 (comment) . I see your point now. I don't see a consensus in the python ecosystem (several python packages and python itself installs with their tests), but these are valid arguments for not including the tests.

( even if that does not matter, for the records, I think my personal preference is to install the tests )

[d-maurer]

Alexander Loechel wrote at 2022-10-11 05:18 -0700:

... so why install the test?

Why not? It cannot be a security issue -- because the source is publicly available. Anybody interested can look there to design a potential attack. Not installing the tests saves a small amount of disk space. But the amount is negligable.

[d-maurer]

Alexander Loechel wrote at 2022-10-11 05:28 -0700:

After even looking more into the pull-request. I veto on releasing any new RestrictedPython version with that merge included. It is a security issue and a absolute nogo. RestrictedPython was always planed at written so that it did not depend on any external package. If the tests lifes inside the package scope and have imports from external packages like pytest, which is even not declared in the setup.py, that is absolutly not accepatble.

Many other `zopefoundation` projects have additional requirements to run the tests -- e.g. `zope.testrunner`. Typically, this is documented via a `tests_require` parameter of `setup.py`. If someone wants to run the tests, he looks there and installs the additional packages.

[loechel]

d-maurer wrote at 2022-10-11 06:17 -0700:

It cannot be a security issue -- because the source is publicly available. Anybody interested can look there to design a potential attack.

@d-maurer it is not a problem in the sense of confidentiality, and I totally agree with you that the test should be public avaliable and even shipped with the code / package. Every user should be able to look at the tests and check them. I also agree that tests are often a good example or hint how to use the software.

It is a regular security issue in the run time purpose. You should not depend on any testing framework or tests in a production deployment, as those adds additional dependencies and could sometimes enable ceratin mocks, so that the intended functionality is not provided.

I know, RestrictedPython is primary used in web development (Zope/Plone), and it is not life critical. But I keep my opinion, coming from developing aviation software. There are several known losts of planes and rockets, because someone shipped test code into production. So I always abstain from including tests into the production code. pytest and other test frameworks work perfectly with a separated test directory, so I use that separation.

[dataflake]

I understand the point but as you say yourself, RestrictedPython issues won't kill anyone. Besides, we ship the tests for all packages in the zopefoundation GH organization. Changing that for one package makes no sense.

[d-maurer]

Alexander Loechel wrote at 2022-10-11 08:23 -0700:

... It is a regular security issue in the run time purpose. You should not depend on any testing framework or tests in a production deployment, as those adds additional dependencies and could sometimes enable ceratin mocks, so that the intended functionality is not provided.

You do not depend on a testing framework in a production deployment -- unless you want to run the tests. No `zopefoundation` package I know installs the testing framework by default -- but most of them install the tests. The tests are there but in order to run them, additional packages need to be installed manually.

I know, RestrictedPython is primary used in web development (Zope/Plone), and it is not life critical. But I keep my opinion, coming from developing aviation software. There are several known losts of planes and rockets, because someone shipped test code into production.

The situation here is quite different: the tests are quite well isolated which makes it unlikely that testing code is executed unintentionally. And usually everything happens on earth with the possibility to take action if something goes wrong.

So I always abstain from including tests into the production code. pytest and other test frameworks work perfectly with a separated test directory, so I use that separation.

In my view, it is an advantage when test/development on one side and production deployments on the other behave mostly identical. This increases the chance to detect problems in the test/development deployment and not in the production deployment. While you may have seen planes and rockets lost due to the accidental execution of testing code, I had some cases of bad experience with problems only observable in the production deployment due to differences between test/development and production (admitted: not involving test code).

[dataflake]

My preferred outcome here would be the following:

• tests are located at src/RestrictedPython/tests to match all other packages we have
• tests use zope.testrunner and not pytest, again to match all other packages we have

That way the maintenance burden is reduced.