Hi,
On Tue, Jun 07, 2016 at 10:30:57AM +0300, Samuli Seppänen wrote:
This is probably correct: the codebase is complex enough to cause
breakage on many types of changes, no matter how carefully the code is
reviewed. This is often because of the sheer number of options and their
invisible interplay. A recent example is the recent persist-tun / WFP
filtering issue, which slipped through all testing and review.
Regression testing using unit tests could possibly help us prevent this
type of breakages.
In particular for *that* example: no way unit testing would have found
that - "full connection testing with multiple remotes and tunnel connection
issues on the first tunnel" would have been needed.
Unit testing might have found the issue that on closing of the WFP in
master, the handle wasn't NULLed, so a second close call would have
caused a crash - if someone had thought of adding a test case for
"call close twice!" *and* ran this on windows.
Unit testing is good, but has no chance to find "full program test"
failures in certain combinations of options and remotes - which is
what t_client is there for, but even that one wouldn't have caught the
WFP thing as it's fairly hard to test for that.
We already have discussed solutions to this:
1) Enable developers to run the ?authoritative? buildslave tests before
submitting patches
The buildmaster can trigger manual builds from a different branch (e.g.
"staging/somebody"). Access to the buildmaster can be granted
selectively to trusted core developers. Non-trivial patches from other
people should probably be tested by one of these core developers before
pushing them to "master". Or we can just accept the fact that "master"
might be broken occasionally and fix the problems promptly.
Haven't we been here before?
- add testing branches to the github repo for (semi-)trusted developers
- teach buildbot to use the github repo
- enable (semi-)trusted developers to push their "I want this tested!"
code to github/openvpn/testing-<user>/ and have buildbot test it
Yes, exactly, this is what I was describing above in other words :).
we can't open this to the world, as the t_client tests need sudo
privileges, so anyone who can push a patch to a testing tree can run
arbitrary commands on the buildslaves ("just build whatever you want
into something called 'openvpn' and run that with sudo from t_client") -
so, (semi-)trusted developers only.
This is not entirely true, because the build steps are hardcoded.
However, I would definitely not open this to the world, because there is
plenty of room for misuse, and Buildbot might have security issues which
could be exploited.
(This *should* have been discussed last monday, but it slipped off the
agenda)
2) Provide developers tooling to quickly (preferred: locally) run
iterations while developing
The Vagrant approach is nice because it will eventually allow developers
to run a fairly extensive smoketests on several various operating
systems with minimal effort:
<https://github.com/OpenVPN/openvpn/pull/45>
Right now the OS coverage is fairly minimal, but more variants can be
added easily, and we're not limited to Linux/Ubuntu like with Travis.
I'm not seeing who is going to maintain all these VMs so every developer
can run the full suite across all OSes...
gert
It seems that a summary of how Vagrant operates is in order here.
Vagrant uses pre-built images as a starting point. These images do not
(and should not) be built by OpenVPN developers. The only things _we_
have to maintain are the Vagrant files, which are basically recipies for
configuring the base boxes into an OpenVPN test VMs.
So, when a developer wants to run the integration tests this is what
happens:
- Vagrant fetches the pre-built VM images from a remote source
- The image is launched into Virtualbox (or other virtualization system)
- Vagrant runs the initialization scripts in the Vagrantfile
- The system is ready to use and stored for future use
The initialization scripts can automatically run integration tests when
the VM launches, or that can be left to the developer.
When the developer wants to rerun the tests, he/she can just launch the
VM with "vagrant up" and SSH in with "vagrant ssh". When the Vagrant
file has changed significantly, the old box can be nuked and a new one
rebuilt in <5 minutes.
Summary: very little maintenance is required for Vagrant. It is not like
buildbot, where we actually have to build the VMs from scratch.
--
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc
irc freenode net: mattock
