More general testing for scripts
Needs RevisionPublic
Actions

Authored by Itms on Feb 19 2017, 4:45 PM.

Details

Reviewers

• leper

Trac Tickets

#4427

Summary

This change allows for more general testing of scripted code. Currently our only tests are under simulation/components whereas we should be able to test global scripts, simulation helpers, etc.

This change also uses the opportunity to address a little note in the code and it fixes #4427. It also removes code duplication in the script interface.

Such a limitation currently blocks (by design) #2951. With this I will be able to write scripted tests that are not scoped to a scripted component but to the entire simulation handling of templates.

Beyond the scope of this patch, we should support other mods than the public one, and we could think about a way to test the GUI.

Test Plan

Run tests and check no tests are actually skipped because of the changes.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 544
Build 867: Vulcan Build	Jenkins
Build 866: arc lint + arc unit

Event Timeline

Itms created this revision.Feb 19 2017, 4:45 PM

Build has FAILED

Updating workspaces.
Build (release)...
In file included from ../../../source/scriptinterface/tests/test_scripts.cpp:17:0:
/mnt/data/jenkins/workspace/phabricator/source/scriptinterface/tests/test_scripts.h: In static member function ‘static bool TestScripts::check_setups(const char*, const VfsPaths&)’:
/mnt/data/jenkins/workspace/phabricator/source/scriptinterface/tests/test_scripts.h:59:84: error: cannot pass objects of non-trivially-copyable type ‘const VfsPath {aka const class Path}’ through ‘...’
    debug_printf("WARNING: Skipping %s tests (can't find %ls)\n", testName, pathname);
                                                                                    ^
make[1]: *** [obj/test_Release/test_scripts.o] Error 1
make: *** [test] Error 2
make: *** Waiting for unfinished jobs....

Link to build: http://jw:8080/job/phabricator/370/
See console output for more information: http://jw:8080/job/phabricator/370/console

Harbormaster failed remote builds in B542: Diff 553!Feb 19 2017, 5:14 PM

Fix the valid GCC complaint ?

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (304 tests)................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (304 tests)................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/371/ for more details.

Harbormaster completed remote builds in B544: Diff 554.Feb 19 2017, 6:46 PM

elexis added a subscriber: s0600204.Feb 22 2017, 6:18 PM

Any chance for getting this with context? (And possibly file moves, though that shouldn't be that much of an issue.)

In D151#5974, @leper wrote:

Any chance for getting this with context? (And possibly file moves, though that shouldn't be that much of an issue.)

Um, it looks like loosing the context comes from using arc on Windows. On top of that, I cannot do the files moves because I'm hitting https://secure.phabricator.com/T10608

I'll create a git branch for eased reviewing.

There you go: https://github.com/na-Itms/0ad/tree/script-testing

In D151#6456, @Itms wrote:

There you go: https://github.com/na-Itms/0ad/tree/script-testing

Much easier to grasp, thank you!

Mostly requesting changes for that one typo, and some rephrasing of that one TODO comment. Code looks fine (not that many changes after one can see that with proper moves).

source/scriptinterface/ScriptInterface.h
253	This should be const, but depending on what is merged first it could also be in D155.
source/scriptinterface/tests/test_scripts.h
31	We use `TODO: Something` normally, the few occurences of `TODO` without a following `:` are due to people merging wip patches of mine without either taking care of those or rewriting them into proper `TODO:`s.
36	Might work, though putting it next to that might lead to the next person adding another file next to those. Also this leads to tests in mod A (on which mod B) depends not being tested when both A and B are mounted, which might hide bugs.
38	That only tests combinations based on the dependency tree, but ignores conflicts (in code behaviour) caused if multiple non-conflicting mods (in the sense that they can be mounted at the same time). This could very well be out of scope for this, but I'm slightly uneasy with this whole TODO comment.
58	Not having an early continue here makes this shorter.
88	We could also find all those setup.js files automatically, and just require a path to the tested folders (`globalscripts/` in this case), and assume `tests/setup.js` or similar.
96	Why does this one have the reversion? The other variants do not have this.
100	s/an/a/