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.

_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to     : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Reply via email to