widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

I'm really happy to see this proposal. I agree there has been too many regressions lately and I definitely agree an automated test suite will help us discover and fix these faster. I haven't really looked much at the tests themselves, but it is really easy and straight-forward to run them at least. :)

I cannot really say much about the c++ code in here, but I've looked over the Python code, and here are some comments:

The first way of loading tests in test_regression.py looks redundant, but I assume this was where the script started out. Can probably be removed now, though.

I think we should consider changing test_loader.discover() pattern to something more like the default one ("test*.py"). Currently it seem to check the __init__ file for test too which is unnecessary, and it could be useful if we want to add some helper files or similar at some point. It would also allow us to..

Please rename the __maps__ directory. (Maybe just nitpick, but I don't really see the need for the underscores except to dodge the test file pattern)

Regarding the hardcoded path to the binary, I think it makes the most sense to provide a command line option to set this, and defaulting to "./widelands" if not present. This is assuming the common invocation is running ./regression_test.py from the root directory¸ which with the command line option would look something like this: ./regression_test.py ../some/other/dir/with/widelands

The iteritems() method for dictionaries doesn't exist in Python3. Looks like items() can be used instead.
http://docs.python.org/3.1/whatsnew/3.0.html#views-and-iterators-instead-of-lists

Also, the parenthesis-less print statements won't work, but they look mostly temporary.

Running with latest trunk, I noticed two of the tests fail (lua_testsuite.LuaTestsuiteInGame and lua_persistence.LuaPersistence), and I think only the former failed yesterday...

-- 
https://code.launchpad.net/~widelands-dev/widelands/regression_testing/+merge/178000
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/regression_testing into lp:widelands.