Page MenuHomeWildfire Games

Scripting tests update
AcceptedPublic

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

Details

Reviewers
wraitii
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
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7322
Build 11925: Vulcan BuildJenkins
Build 11924: arc lint + arc unit

Event Timeline

Itms created this revision.Sat, Apr 27, 11:38 PM
Owners added a subscriber: Restricted Owners Package.Sat, Apr 27, 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.Fri, May 10, 12:25 PM
Stan added inline comments.
binaries/data/mods/_test.sim/simulation/components/test-hotload1.js
23

Missing spaces.

binaries/data/mods/_test.sim/simulation/components/test-modding2.js
2

Missing spaces between operators.

source/scriptinterface/tests/test_ScriptInterface.h
267

Anyway to not have the hardcoded js ?

Itms marked an inline comment as done.Sun, May 19, 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

Sure I'll change that.

source/scriptinterface/tests/test_ScriptInterface.h
267

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.Sun, May 19, 10:43 AM
source/scriptinterface/tests/test_ScriptInterface.h
267

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.Sun, May 19, 10:52 AM
Itms added inline comments.
source/scriptinterface/tests/test_ScriptInterface.h
267

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.Sun, May 19, 10:55 AM
source/scriptinterface/tests/test_ScriptInterface.h
267

Sure was just as suggestion :)

Itms added inline comments.Sun, May 19, 11:01 AM
source/scriptinterface/tests/test_ScriptInterface.h
267

Yeah no problem, I'm just explaining it in detail so that you learn new things ๐Ÿ™‚

Stan added inline comments.Sun, May 19, 11:03 AM
source/scriptinterface/tests/test_ScriptInterface.h
267

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

wraitii added inline comments.Sun, May 19, 11:41 AM
source/scriptinterface/tests/test_ScriptConversions.h
133

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

Itms added inline comments.Sun, May 19, 11:50 AM
source/scriptinterface/tests/test_ScriptConversions.h
133

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.Sun, May 19, 11:53 AM
source/scriptinterface/tests/test_ScriptConversions.h
133

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

Itms added inline comments.Sun, May 19, 12:00 PM
source/scriptinterface/tests/test_ScriptConversions.h
133

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.Sat, May 25, 10:09 AM

Use the global directly and commit.

This revision is now accepted and ready to land.Sat, May 25, 10:09 AM