[WIP][RFC] Python test suite

[D4N]

As promised, here is a preview of the test suite I have been working on to replace the shell scripts & binary files we have in test/. This is still work in progress, so please do not merge.

Rationale

This rewrite aims to address some of the shortcomings of the current regression test suite:

• issues with cross-platform portability
• very hard to manage with git due to the usage of binary files
• the current testsuite only checks the combined stdout & stderr (and ignores the return value), also if exiv2 should hang during the processing of a file, the testsuite will hang too

Test cases

For the python test suite each test case is defined as via a yaml file. For example:

---
name: Test for CVE-2017-14857 / Github bug 76
actions:
  - file_path: 010_bad_free
    action: "{exiv2} {file_path}"
    stdout: ""
    stderr: |
      Exiv2 exception in print action for file {file_path}:
      invalid memory allocation request
    retval: 1

Short explanation of the configuration file format:

• every yaml file begins with ---
• each test case must have a name and a list of actions (there can be multiple actions)
• each action must be a dictionary providing the following keys: action, stdout, stderr and retval
• additional keys in an action ending with _path are treated as paths relative to the configuration file's location

The test suite will process this file, find all the actions it should run and substitute all strings enclosed in curly braces with either values defined in the main config file (more about that later, here that would be {exiv2}) or with additional keys of this action (e.g. relative paths, like file_path in this example). This substitution is done using python's format() function, thus the curly braces.

The idea behind this is, that one can create a directory somewhere under test/, put a yaml config file and additional files (like images) in it and let the testsuite figure out all the business with the correct paths. Also, it lets you define "global" paths (exiv2 here), that are defined in the main config file, which are automatically expanded for each test case's location.

For the above example, the testsuite would run exiv2 010_bad_free via Python's subprocess module, compare the stdout, stderr & return value to the provided values and show the differences via diffs (just like the current test suite).

Testsuite

The testsuite itself can be configured via a yaml file too:

---
suite_root: .
timeout: 0.01
paths:
  exiv2: ../build/bin/exiv2
  exiv2json: ../build/bin/exiv2json

with the following configuration options:

• suite_root is the relative path (from the configuration file) to the directory under which the suite should recursively search for test cases
• timeout is a time in seconds which each action can take at most before being killed
• paths is a dictionary for relative paths that are expanded to absolute paths for each test case (so that you only have to write {exiv2} in your test case and don't have to worry about your location on the file system or the value of $PATH)

Additional notes

I have added also two test cases as an example, you can run them via python3 tests/runner.py tests/config.yml. One of them will produce an error because one of the test case configuration files has a wrong return value (which I have set for illustration).

The test suite treats directories as a "set" or "group" of test cases and will group their execution together. Thus the tests could be grouped hierarchically, as I have indicated with the example bugfixes/github. There could be another directory bugfixes/redmine for older regression tests, conversions/ for conversion tests, etc.

For the curious: this is inspired by ansible.

Possible improvements / shortcomings

• @piponazo suggested the usage of Python's builtin unit test facility to accomplish this. I have not considered this (completely forgot that that would be an option) but I am unsure whether Python's unittest could actually help here (as the individual test cases are not python code but configuration files).
• It is probably slower than the current implementation and very much untested (will probably not work with python 2).
• The documentation is non-existent
• Requires PyYAML as an external dependency. If we do not want another dependency for development, we could use python's ConfigFileParser. Originally I wanted to use json (as it is builtin the standard python library) but it does not support multi-line strings, which would make the config files ugly (think long strings with \n or list of lines). Ultimately I choose yaml because I am familiar with it from ansible, but switching to something else is trivial as there are only two places where the yaml module is used.

[piponazo]

Hi @D4N , first of all I have to say that this is an excellent piece of work! I really like the idea of defining tests with text files. I do not know if something similar could be achieved with the unittest module since I do not have too much experience there yet (I just started to use it recently).

Some of my thoughts after reading your description (and before starting with the code review):

• I would not worry to much about losing a bit of speed when running these system tests. I rather prefer to have a system like this that is easy to maintain and to understand (even for people unfamiliar with python) than some outdated bash script code.
• About python2 / python3 / PyYAML . If we will require to have python to run these system tests, I would go directly for python3 rather than choosing python2. These is just something that the exiv2 developers will use for asserting the code quality. Python3 is also available in the travis-ci and appveyor images. However, I would try to avoid to add dependencies in external python modules (PyYAML) unless it is strictly necessary.
• Regarding the documentation, you could copy and paste a big part of this nice description into the source code to explain how this new system work 👍

I will give it a try in the next days and share my review about the code.

[piponazo]

When I run the tests and there is no errors (I modified the error that you introduced intentionally) I get an output like this one:

17:21 $ python3 tests/runner.py tests/config.yml 
Running: bugfixes/github
..

This is not telling us if the tests passes or even if they run at all. If we do not use a python testing framework, we would need to implement something for summarizing what happened after running the tests.

[piponazo]

We could add here a default value to the main configuration file (that should be the same in most of the cases). What do you think ?

[D4N]

That won't hurt imho.

[piponazo]

All this main function is duplicated in the runner.py file. I am not very experienced with Python and I am not sure if this is intended or not, but I guess that if you want to have a main function in both files you can extract out the common code into a separated file.

[D4N]

Nope, not required and not intended. Just a copy + paste fail, I simply forgot to remove the code here.

[clanmills]

I agree with everything said about the great architecture being used here. I very much like the use of Python and it gets us away from bash (which requires using cygwin on Windows). I wish we didn't have to choose between python2 and python3, however if we must choose - let's choose python3.

Python has a multi-line string """........"""

The location of executables in the current test suite uses the (optional) environment string EXIV2_BINDIR. This was added by Volker around 2010. For sure, we should avoid using $PATH to find executables. It's important to externally specify the bin and not hard wire it to be ../build/bin. When I build (using contrib/cmake/msvc/cmakeBuild), I can build into 56 different directories (for different versions of MSVC, 32/64, static/shared, debug/release)

I wouldn't worry about performance until it is clearly an issue. If the running time for the whole suite is of the order of the build time, that's fast enough. Let's get test/bugfixes.sh converted to this architecture and compare performance.

The current test suite is in /test (and unitTests are in /unitTests). Can we keep /test alive for now and put this development in /tests. We'll deprecate /test with v0.26.1 and remove him when we're smoothly using /tests

I'm looking forward to using this and I'll give you feedback when I used it.

Very good work indeed. Thank You very much for working on this (and all the other great stuff you are doing).

[piponazo]

[suggestion] I think you can refer to these "whole program tests" as "system tests".

[D4N]

I have used your wording, as I found my odd in the first place.

[piponazo]

When I looked at this it was a bit strange for me to have the possibility of having variables with several words. Would you re-consider to have a restriction for the variable names (so that a variable must contain exactly 1 word) ? In that case I would turn this file in something like:

error_57_message: invalid memory allocation request
exiv2_exception_msg: Exiv2 exception in print action for file

[D4N]

Multi word variables are a feature of Python's ConfigParser module and afaik there is no way how to forbid it in the ConfigParser itself. I could of course implement a check that would reject all variables with whitespaces in them, but to be honest I don't find that too bothering, as the variables either appear before the delimiter : or enclosed in curly braces.

I'll keep it in mind and will take a look how hard enforcing this would be.

[D4N]

Ok, I think I have hit yet another dead-end with this. As you might have seen in the source examples and the documentation, I have removed the yaml requirement and instead replaced it with Python's buitin ConfigParser which turned out to have some nice features, like automatic variable substitution via ${varname}.

I just though this will could be completed, but then I encountered a pretty bad show stopper: ConfigParser strips away leading and trailing whitespaces in variable values (this seems to be builtin into the module without a switch). This means that if we would like to define an expected stdout of (please note the trailing whitespace after the colon):

Camera model    : 

in a INI style file like this:

stdout:Camera model    : 

the variable stdout is saved without the trailing whitespace in the ConfigParser.

I am afraid that ConfigParser is also unsuitable for this, as some output of exiv2 produces trailing whitespaces.

I think I am going to try @piponazo's idea with using Python's builtin unittest module. If it works like I currently imagine it, it should require minimal Python knowledge to be run.

[D4N]

So, I have once again rewritten the test suite, now according to @piponazo original idea to use Python's unittest module, which in hindsight was the best approach.

I have updated all example test cases to the new structure and updated the Readme, so hopefully it is at least a bit understandable how this works.

I have also tried to add the test suite to the CI, which was successful on travis, but appveyor throws quite odd errors (the Python installation there doesn't seem to have the builtin configparser module, which is rather odd).

Further suggestions/ideas? If you have the time also please test it a little bit and tell me if you miss some features (I am thinking about adding the option to save environment variables, so that we can run e.g. run exiv2 with gprof and without).

[piponazo]

I tried the testing suite in two platforms with the following results:

• Ubuntu 17.04. It worked like a charm with python3.
• Windows 10. The tests failed because they cannot find the executable path:

pipon@DesktopWin10 MINGW64 /f/projects/exiv2/tests (D4N-python_test_suite_wip)
$ python -m unittest
EEE
======================================================================
ERROR: setUpClass (bugfixes.github.test_CVE_2017_14680.CVE_2017_14680)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "F:\projects\exiv2\tests\suite.py", line 89, in setUpClass
    rel=rel_path)
ValueError: Path replacement for exiv2: F:\projects\exiv2\build\bin\exiv2 does not exist (was expanded from ../build/bin/exiv2)

======================================================================
ERROR: setUpClass (bugfixes.github.test_CVE_2017_14857.CVE_2017_14857)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "F:\projects\exiv2\tests\suite.py", line 89, in setUpClass
    rel=rel_path)
ValueError: Path replacement for exiv2: F:\projects\exiv2\build\bin\exiv2 does not exist (was expanded from ../build/bin/exiv2)

======================================================================
ERROR: setUpClass (bugfixes.redmine.test_1305.Issue1305Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "F:\projects\exiv2\tests\suite.py", line 89, in setUpClass
    rel=rel_path)
ValueError: Path replacement for exiv2: F:\projects\exiv2\build\bin\exiv2 does not exist (was expanded from ../build/bin/exiv2)

----------------------------------------------------------------------
Ran 0 tests in 0.016s

FAILED (errors=3)

As you can notice, the .exe extension is missing.

[piponazo]

We will need to have the equivalent for mac. Since mac it is right now disabled, could you add a #TODO comment ?

[D4N]

I think I am going to remove the Python 2 run altogether, and with it this installed package.

[piponazo]

I just want to understand. The objective here is to check that we can run the tests with python2 and python3 ?

I do not see why to keep care of 2 python versions. This would make sense in a project which had someting implemented with python2 and wants to move to python3. Since we are introducing this new testing framework now, I would just use python3.

[D4N]

Originally I though about only supporting Python 3, but then it turned out that the test suite ran on Python 2 without any modifications, so I just wanted to test it on Travis. I don't think we need to keep the Python 2 run.

[piponazo]

Instead of deactivating the virtual environment you could install the unittest module with pip in the virtualenvironment we use for conan.

This would have the advantance of caching the installation of the module in the travis-ci cache. It will be only installed once and reused in the future (saving time in each build job in travis-ci)

[D4N]

I'll take a look at that. But probably we don't need that if I remove the python 2 run, as the Python 3 run doesn't need any additional packages.

[piponazo]

Here we are forcing to the developers who want to run the python tests to always use the build folder to build the project. It is not something critical (I would not block the PR just for this), but it would like to have the possibility of using any name for the build folder.

[D4N]

Yeah, I know. Combined with the fact that the config file's path is currently hardcoded, makes it really inconvenient to change the paths here (as you'd have to edit the file that is under version control).

I was thinking about adding a section [ENV] to the config files where you could specify which variables you would like to extract from the environment variables. And then you could use configparsers interpolation to make it more dynamic. This could also fix the missing extension on windows. This could look like this for example:

[ENV]
exiv2_path: EXIV2_PATH
exiv2_file_type: EXIV2_FILE_TYPE

[paths]
exiv2: ${ENV:exiv2_path}/exiv2${ENV:exiv2_file_type}

If an environment variable is unset, then it would expand to "". If we then provide some defaults in the [DEFAULT] section then this could work.

[piponazo]

[suggestion] I had to read the code around and add a print statement to understand the meaning of this variable. Could we rename it to something like test_folder, test_cwd ?

I think we should not skimp on the lenght of variable names if it affects to the code readability.

[D4N]

I'll improve them, the variable names don't really tell you whats behind them.

[piponazo]

[question] Could you explain way you are using here the threading module ? Is it for achieving parallelization ? or is it needed for something else ?

[D4N]

This is purely to have a portable way how to stop the running process after the timeout has expired. The problem is that subprocess.Popen did not have the timeout parameter itself until very recently (especially not in Python2). But if you use the threading module, you can stop a running process via process.kill() from a different thread. That's where the threading module comes in, as the Timer runs in a different thread.

While it would be nice to run the tests in parallel, I am afraid Python's unittest framework does not support that.

[D4N]

I have updated the test suite and added the environment variable extraction which I mentioned in one of the comments. I have documented this in the README.md file, if you want to take a look at the details (you can find the rendered version here).

The inclusion of environment variables allows us to set platform differences (like different file extensions of executables) via the environment. And now you can also override the default directory where the exiv2 binaries reside in.

The tests still fail on Appveyor, although it claimed that they pass. The problem is that Appveyor's python 3.6 doesn't have the subprocess module installed, albeit that being a built-in module. That is rather odd but something similar happened with python 2 where ConfigParser was missing. Does someone know what on earth is wrong with their python installation? Or is this a thing on Windows? I certainly hope not.

[D4N]

I just rechecked the Appveyor error log and I have had a brain-fart: the problem is not that the module subprocess is missing but that the subprocess module cannot find the path to the test cases.

Could someone who owns a Windows machine give this a try?

[clanmills]

I'll have a look at this and get back to you. Thanks for working on this.

[D4N]

I think I was able to fix the problems on windows, at least Appveyor seems not to complain anymore. The main issue was that shlex.split would remove all \ from Windows pathnames resulting in invalid paths. And then the usual \r\n vs \n stuff.

[D4N]

I'll be damned. Just as I submitted the comment, I see Appveyor light up red. I just re-pushed a commit with a different commit message that was working previously... Also, for some obscure reason only one of the regression tests is failing and not all of them.

Robin, could you take a look if this is reproducible on your Windows machine?

[clanmills]

I'm approving this without reviewing the code, just so the merge can proceed.

I've built/tested master/head with contrib/cmake/msvc using the (DOS) command: cmd/c "vcvars 2017 64 && cmakeBuild --build --release --test" and that looks OK (yeah, that's great).

I'm having sproadic (occasional) trouble with exiv2-test.sh. It works on my Mac desktop and fails on my Mac laptop. It's failing on Cygwin, MSVC/ and contrib/cmake/msvc. Perhaps we should disable that test for the moment.

However, I'm not sure what I'm looking for here. Is this a build issue with in appveyor.yml (which I haven't looked at yet), or with the python test code (which I haven't looked at yet either). Gosh, I have to get back "up to speed" with all the great work you and Luis have contributed.

We will easily get this fixed. Happy to do 1-to-1 on Slack (or Skype or something). We're at a Christmas Party on Thursday evening, otherwise mostly home.

[D4N]

So, I just pushed a tiny commit to fix the OSX build and now Appveyor is green again. That is quite odd.

I think I would add a few additional test cases from the regression test suite to see if any further problems arise and not merge this yet. Also, before merging, I'll squash the commits as there is a lot of test-commits and obsolete ones present.

[piponazo]

Great work @D4N . I will try to run the tests on Windows probably this evening but feel free to merge now and we can always tweak some things later ;)

[D4N]

I'd like to add at least some tests before we merge this. In case this turns out to be unstable on Appveyor, it would become a huge pain.

Also, we could also use this opportunity to add git LFS for the reproducers. Whats your opinion concerning that?

[piponazo]

Humm there are many things to take into account about git LFS.

In first place, just enabling it is not enough to "solve" the issue with binarys in our repo. The files are already here (in the git history) and we would need to clean the git history after moving the binary files to LFS, to reduce the size of this repository.

In my opinion I think it would be better to move the binary files to a git LFS repo, but probably to a different one. Then, we could have some python code to pull things from that repo before running the python test suite. Anyways, those binary files are just used by these system tests.

These are very rough ideas (I am at work right now 😛 ). We would need to discuss much more about this, present several ideas, and decide something together.

About the testing, I will give you some feedback this evening.

[clanmills]

I don't know what LFS is (Less Fun Stuff?), however we already have a way of dealing with large binary files. There's an SVN repos: svn://dev.exiv2.org/svn/testdata which has EPS and VIDEO files. The test suite pulls them down on demand (using the command svn export).

612 rmills@rmillsmbp:~/gnu/exiv2/trunk $ svn list svn://dev.exiv2.org/svn
branches/
tags/
team/
testdata/
trunk/
613 rmills@rmillsmbp:~/gnu/exiv2/trunk $ 

This was introduced because Luminance HDR complained that our svn repos was 75mb (with 50mb of test files).

[D4N]

@clanmills git LFS stands for git large file storage, it is one of the two approaches I know of that ease the tracking of binary files with git less painful (the other being git annex). Afaik with git LFS you have a separate server for all your binary files and you only tell git that such a file exists and how it can download it. But git does not store the snapshots of your binaries at every commit touching them. In theory we could also move the files from the svn repo to git lfs at some point.

@piponazo I know that we won't remove the binaries from the history, and I wouldn't want to do that. We shouldn't rewrite the history of the repository. But the issue with the new test suite is that it will require to create copies of all the binary test files under /tests/ which will increase the repos size. Also, if we would just move them (don't know how git handles them), we would have to adapt the infamous bugfixes-test.out (and its cousins under /test/) at every commit where we port a test case, thereby increasing the repos size (probably) even more.

[D4N]

Gentlemen, unfortunately this PR has been open for quite some time (my apologies, I am a perfectionist...). I have added some additional features to the new test suite that I felt were necessary which allowed me to port tiff-test.sh (this should fix #190).

I'll update the documentation tomorrow or the day after and then we can hopefully get this merged.