Issue 3985 - Unified test runner for sitescripts

I configured tox to run the part of the tests that doesn't need anything not covered by tox/pip and added a Vagrantfile that allows to run the rest of the tests too. Everything is documented in the README.

[mathias, 2016-05-23 22:25:33]

Quite promising, but IMHO there is a step missing in the concept, namely some
sort of build out of /vagrant into /opt/sitescripts or setup.py install
somewhere.. see below.

https://codereview.adblockplus.org/29342955/diff/29342956/README.md
File README.md (right):

https://codereview.adblockplus.org/29342955/diff/29342956/README.md#newcode108
README.md:108: vagrant@debiancontrib-jessie:/vagrant$ tox -e
filtehits,notifications
Do you really want a binary image from an un-trusted source being able to write
back files you could potentially execute on your host system? Even if not
malicious by intention: I do not see how one would trust a test environment with
potentially broken code and potentially just slightly different executable
objects that much.

https://codereview.adblockplus.org/29342955/diff/29342956/README.md#newcode120
README.md:120: $ find . -name '*.pyc' | xargs rm
After addressing my comment above, you could transform this section into a
lecture about not mixing up the environments in the first place, and what to do
if it happened by accident.

https://codereview.adblockplus.org/29342955/diff/29342956/Vagrantfile
File Vagrantfile (right):

https://codereview.adblockplus.org/29342955/diff/29342956/Vagrantfile#newcode8
Vagrantfile:8: config.vm.provision "shell", inline: <<-end
If the script wouldn't be inline one could invoke it anywhere. In Travis, for
example.

https://codereview.adblockplus.org/29342955/diff/29342956/Vagrantfile#newcode9
Vagrantfile:9: set -e
Please make sure that multiple invocations of "vagrant provision" do not break
the bootstrap script.

https://codereview.adblockplus.org/29342955/diff/29342956/Vagrantfile#newcode13
Vagrantfile:13: sudo debconf-set-selections <<< 'mysql-server
mysql-server/root_password password 123'
Please always use "changeme" globally as default test password, don't invent new
ones to remember, thank you!

https://codereview.adblockplus.org/29342955/diff/29342956/Vagrantfile#newcode17
Vagrantfile:17: mysql -uroot -p123 -e "create user 'test_user'@'localhost'
identified by 'test_password'"
Same here.. ;-)

https://codereview.adblockplus.org/29342955/diff/29342956/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29342955/diff/29342956/tox.ini#newcode18
tox.ini:18: # Depends on MySQL client (TODO: and server?).
Probably libmysqlclient-dev plus maybe some common base package. This TODO
should vanish before this patch-sets lands, with Vagrant you can easily find out
what's missing.

[Sebastian Noack, 2016-05-24 07:58:37]

https://codereview.adblockplus.org/29342955/diff/29342956/README.md
File README.md (right):

https://codereview.adblockplus.org/29342955/diff/29342956/README.md#newcode97
README.md:97: The tests dependent on the database are covering the packages
`filterhits` and
Nice idea to use Vagrant for tests with non-python dependencies. Just some
thoughts on that approach:

1. The way you intend those tests to run, couldn't one simply setup the machine
with the infrastructure repository, rather than having a separate Vagrantfile
for testing in here?
2. If we integrate Vagrant for the tests here, couldn't we do it the other way
around, that people just have to run tox, which then setups the VM and runs
tests in there if necessary?

https://codereview.adblockplus.org/29342955/diff/29342956/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29342955/diff/29342956/tox.ini#newcode27
tox.ini:27: commands = py.test sitescripts/hg/test
Is there any benefit on using py.test to invoke standard unittests?

[Vasily Kuznetsov, 2016-05-24 18:15:36]

Since we're getting rid of `filterhits`, I excluded its tests from the
configuration. I also made it that `sitescripts.ini` used for testing doesn't
include irrelevant web handlers and that eliminated dependencies on MySQLdb and
M2Crypto (thanks, Sebastian). The result is that all tests can now run without
any additional dependencies not managed by Tox, so I just moved them into the
same Tox environment to get one combined report. I also added test coverage
reporting for those submodules of `sitescripts` that do have tests.

This seems to have made most of the comments irrelevant since we don't have
Vagrantfile anymore and the documentation became much shorter. Sorry for your
time :/

https://codereview.adblockplus.org/29342955/diff/29342956/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29342955/diff/29342956/tox.ini#newcode27
tox.ini:27: commands = py.test sitescripts/hg/test
On 2016/05/24 07:58:36, Sebastian Noack wrote:
> Is there any benefit on using py.test to invoke standard unittests?

Nicer reporting. Also it allows having a single report for multiple tests. It
can also measure coverage.

[Sebastian Noack, 2016-05-25 07:51:36]

Good job. That is much better. Also mind updating the issue? And any reason why
the review is closed? :/

https://codereview.adblockplus.org/29342955/diff/29343014/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29342955/diff/29343014/tox.ini#newcode27
tox.ini:27: bash -c "sed -e 's/sitescripts\.[a-zA-Z_]*\.web.*/# &/'
.sitescripts.example >.sitescripts.test"
Perhaps we should only exclude the web handlers that will cause an ImportError.
For the other handlers, it's a nice side effect to have implicitly tested
whether they can be imported at least.

https://codereview.adblockplus.org/29342955/diff/29343014/tox.ini#newcode29
tox.ini:29: py.test \
I think we should also run flake8, probably with a quite long ignore list. But
at least the whitespaces should comply now, and we should make sure that we
don't regress there.

[Vasily Kuznetsov, 2016-05-25 15:17:41]

https://codereview.adblockplus.org/29342955/diff/29343014/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29342955/diff/29343014/tox.ini#newcode27
tox.ini:27: bash -c "sed -e 's/sitescripts\.[a-zA-Z_]*\.web.*/# &/'
.sitescripts.example >.sitescripts.test"
On 2016/05/25 07:51:36, Sebastian Noack wrote:
> Perhaps we should only exclude the web handlers that will cause an
ImportError.
> For the other handlers, it's a nice side effect to have implicitly tested
> whether they can be imported at least.

Done.

https://codereview.adblockplus.org/29342955/diff/29343014/tox.ini#newcode29
tox.ini:29: py.test \
On 2016/05/25 07:51:36, Sebastian Noack wrote:
> I think we should also run flake8, probably with a quite long ignore list. But
> at least the whitespaces should comply now, and we should make sure that we
> don't regress there.

Done.

Not sure if this is very meaningful at this point given the length of the ignore
list but at least we can sort of measure progress now. We should probably start
by removing the less common errors and the more "bad" errors.

[Sebastian Noack, 2016-05-25 19:17:44]

LGTM