Page MenuHomeWildfire Games

Scripting tests update
ClosedPublic

Authored by Itms on Apr 27 2019, 11:38 PM.

Details

Summary

This is taken from D1844 with an extra update of the JS int limits, and some whitespace. It mainly tests features that are useful for modding.

Test Plan

Check that tests pass.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Itms created this revision.Apr 27 2019, 11:38 PM
Owners added a subscriber: Restricted Owners Package.Apr 27 2019, 11:38 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1301/display/redirect

I'm assuming this is post SM45 ?

Stan added a subscriber: Stan.May 10 2019, 12:25 PM
Stan added inline comments.
binaries/data/mods/_test.sim/simulation/components/test-hotload1.js
23 ↗(On Diff #7877)

Missing spaces.

binaries/data/mods/_test.sim/simulation/components/test-modding2.js
2 ↗(On Diff #7877)

Missing spaces between operators.

source/scriptinterface/tests/test_ScriptInterface.h
267 ↗(On Diff #7877)

Anyway to not have the hardcoded js ?

Itms marked an inline comment as done.May 19 2019, 10:38 AM

I'm assuming this is post SM45 ?

No, this should be in before, so that SM45 is shown to not break those (it doesn't).

binaries/data/mods/_test.sim/simulation/components/test-hotload1.js
23 ↗(On Diff #7877)

Sure I'll change that.

source/scriptinterface/tests/test_ScriptInterface.h
267 ↗(On Diff #7877)

What do you mean by hardcoded? This is not supposed to be moddable or anything, this is just the engine testing that it can run some specific JS code and yield the expected results from it.

Stan added inline comments.May 19 2019, 10:43 AM
source/scriptinterface/tests/test_ScriptInterface.h
267 ↗(On Diff #7877)

Well as elexis said here https://wildfiregames.com/forum/index.php?/topic/25393-moving-components-schemas-to-xml-files/, having two languages in the same file is an anti pattern, and since you seemed to be loading scripts in Component manager I was wondering why you didn't do it as well.

man.LoadScript(L"simulation/components/test-modding1.js")

In binaries/data/mods we have, _test.dae, _test.sim etc we could have _test.scriptinterface, and there you could just tweak the js code without having to recompile. Just an idea.

Itms marked an inline comment as done.May 19 2019, 10:52 AM
Itms added inline comments.
source/scriptinterface/tests/test_ScriptInterface.h
267 ↗(On Diff #7877)

No, this is a test file for the script interface, which parses and executes JS code in the C++ engine, so having the two languages in the file is perfectly natural. In the component manager, I'm testing the ability to load scripts and overload them when modding/hotloading. If I moved this code to a JS file, I would be testing the loading from the file and the snippet I actually want to test, which would make the test non-unitary (i.e. if it fails you don't know what failed).

We could have a _test.scriptinterface mod indeed, but we would use it for different things like tests for globalscripts hotloading when we have that, testing the fix for #5376 when we have it, etc.

Stan added inline comments.May 19 2019, 10:55 AM
source/scriptinterface/tests/test_ScriptInterface.h
267 ↗(On Diff #7877)

Sure was just as suggestion :)

Itms added inline comments.May 19 2019, 11:01 AM
source/scriptinterface/tests/test_ScriptInterface.h
267 ↗(On Diff #7877)

Yeah no problem, I'm just explaining it in detail so that you learn new things 🙂

Stan added inline comments.May 19 2019, 11:03 AM
source/scriptinterface/tests/test_ScriptInterface.h
267 ↗(On Diff #7877)

Thanks ! Also it will be useful if someone ask me the question in the future :)

wraitii added inline comments.May 19 2019, 11:41 AM
source/scriptinterface/tests/test_ScriptConversions.h
133 ↗(On Diff #7877)

If this is valid for SM38, why is this changing?

Itms added inline comments.May 19 2019, 11:50 AM
source/scriptinterface/tests/test_ScriptConversions.h
133 ↗(On Diff #7877)

This should have been changed in a previous SM upgrade, yes. I don't know which one bumped jsval limits to the current values, but the new cassert will allow us to not miss future changes.

Note that this is valid because the old value is now completely irrelevant and is a i32 value like any other.

wraitii added inline comments.May 19 2019, 11:53 AM
source/scriptinterface/tests/test_ScriptConversions.h
133 ↗(On Diff #7877)

Ah, right. Any reason yo not use JSVALA_INT_MAX directly here?

Itms added inline comments.May 19 2019, 12:00 PM
source/scriptinterface/tests/test_ScriptConversions.h
133 ↗(On Diff #7877)

Ah, it could be used for the first argument yes indeed, and would remove the need for the cassert and all those comments. I basically kept things as they were, and it's quite readable this way, but doing what you suggest is probably better.

wraitii accepted this revision.May 25 2019, 10:09 AM

Use the global directly and commit.

This revision is now accepted and ready to land.May 25 2019, 10:09 AM
Itms updated this revision to Diff 8794.Jul 9 2019, 8:16 PM

Address comments and use the opportunity to test the new Jenkins setup.

Vulcan added a comment.Jul 9 2019, 8:34 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3/display/redirect

Vulcan added a comment.Jul 9 2019, 8:39 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4/display/redirect

Vulcan added a comment.Jul 9 2019, 8:52 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/7/display/redirect

Itms updated this revision to Diff 8996.Fri, Jul 19, 5:29 PM

Fix an integer overflow.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/scriptinterface/tests/test_ScriptConversions.h
|  31| class·TestScriptConversions·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestScriptConversions:' is invalid C code. Use --std or --language to configure the language.

source/scriptinterface/tests/test_ScriptInterface.h
|  26| class·TestScriptInterface·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestScriptInterface:' is invalid C code. Use --std or --language to configure the language.

source/simulation2/tests/test_ComponentManager.h
|  41| class·TestComponentManager·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestComponentManager:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/135/display/redirect

This revision was automatically updated to reflect the committed changes.