Paths

Table of Contentst

Differential D871

Deepfreeze entity and template in simulation components tests
ClosedPublic
Actions

Authored by elexis on Sep 5 2017, 12:55 AM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP20134: Mark simulation component template and entity properties as read-only in…

Trac Tickets

#4759

Summary

As seen in rP20015 / D792, the JS simulation component tests don't freeze the template and entity property (while the ingame simulation components actually do, as proven by the bug fixed in rP19623).
The missing freeze allowed unintentional modification of the template object and messed up a test, which was later fixed in that commit.
To avoid repetition of that issue, we can now simply call deepfreeze on these two properties and mark the properties as non-configurable.

I didn't read and comprehend rP20015 in it's entirety, but I assume the two clone calls can be removed, as any modification by a user thereof would now fail loudly.
The function returns a read-only object, so if a clone is needed, it can still be done by the user.

Test Plan

Notice that the freezing works as intended by not removing or adapting the tests as proposed.

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

elexis created this revision.Sep 5 2017, 12:55 AM

elexis updated the Trac tickets for this revision.Sep 5 2017, 1:00 AM

The Object.defineProperties call seems quite pedantic in making sure the behaviour is the same. Just saying, not complaining at all.

It might be nice to have some test that actually makes sure that this keeps working (having properties readonly in those tests, similar to how we check that for C++ in test-param.js and test-entityid.js (in _test.sim)).

Do we actually need the || null? Anyway I'll leave checking the sanity of the js code to fatherbushido.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  66| »   »   »   "GetOwner":·()·=>·player
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  70| »   »   »   "GetOwner":·()·=>·enemyPlayer
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  74| »   »   »   "GetOwner":·()·=>·friendlyPlayer
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  81| »   »   "GetPosition":·()·=>·new·Vector3D(4,·3,·25),
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  82| »   »   "GetRotation":·()·=>·new·Vector3D(4,·0,·6),
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  83| »   »   "GetTurretParent":·()·=>·INVALID_ENTITY,
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_Attack.js
| 166| »   attackComponentTest(className,·true,·(attacker,·cmpAttack,·defender)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/Attack.js
| 199| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 239| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 533| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 585| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/483/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1968/ for more details.

Harbormaster completed remote builds in B3011: Diff 3493.Sep 5 2017, 5:15 AM

In D871#34020, @leper wrote:

Just saying, not complaining at all

I take it as a complement (also just want those defaults to be more transparent to the reader)

Do we actually need the || null?

We do (remove and try to attack chicken or run the tests)

test

Proposing to append this test to setup.js so that no C++ changes are needed and all other files prefixed with test_ continue to correspond to existing simulation components.

{
	// Test the test setup
	Engine.RegisterInterface("TestSetup");

	let TestSetup = function () {};
	TestSetup.prototype.Init = function() {};

	Engine.RegisterSystemComponentType(IID_TestSetup, "TestSetup", TestSetup);
	let cmpTestSetup = ConstructComponent(SYSTEM_ENTITY, "TestSetup", { "property": "value" });

	let expectException = function(func) {
		try {
			func();
			Engine.TS_FAIL("Missed exception");
		} catch (e) {}
	}

	expectException(() => { cmpTestSetup.template = "replacement forbidden"; });
	expectException(() => { cmpTestSetup.template.property = "modification forbidden"; });
	expectException(() => { cmpTestSetup.template.other_property = "insertion forbidden"; });
	expectException(() => { delete cmpTestSetup.entity; });
	expectException(() => { delete cmpTestSetup.template; });
	expectException(() => { delete cmpTestSetup.template.property; });

	TS_ASSERT_UNEVAL_EQUALS(cmpTestSetup.template, { "property": "value" });
}

Notice in stict mode, functions can only be defined at the top level for some reason, hence the let.
(Still waiting to catch a regression in the game with these)

I take it as a complement (also just want those defaults to be more transparent to the reader)

So are you now complete or was that a typo?

test

Proposing to append this test to setup.js so that no C++ changes are needed and all other files prefixed with test_ continue to correspond to existing simulation components.
[code]

Looks good, but shouldn't we also complain the exception message? (Not that that will do anything but require someone to change the test if some future SM version has other (possibly better) error messages.)

(Still waiting to catch a regression in the game with these)

Try some SM upgrade and you will like all these random tests for strange details a lot more.

In D871#34238, @leper wrote something equal to:

should we also complain the exception message in the test?

There is no exception message, since there is no exception in case the test fails.
If we want to make it easier to debug (in particular giving the line number of the actual test that failed, I'd offer an Engine.TS_FAIL("Missed exception at " + new Error().stack);, resulting in nearly what we intend to print:

In TestComponentScripts::test_scripts:
../../../source/test_setup.cpp:134: Error: Test failed: L"Missed exception at expectException@simulation/components/tests/setup.js:127:44\n@simulation/components/tests/setup.js:131:2\n"
<one repetition for about every testfile>

Getting rid of the repetition is only possible by creating a new file.
So just adding a new file that doesn't break the test_ActualComponentName pattern and while at it, also being able to use regular functions (still not getting a filename without the custom exception stack).

In D871#34238, @leper wrote:

I take it as a complement (also just want those defaults to be more transparent to the reader)

So are you now complete or was that a typo?

According to https://en.wiktionary.org/wiki/compliment and https://en.wikipedia.org/wiki/Doublet_(linguistics) those are etymological twins, so I meant whatever was the one making the most sense. :P

In D871#34238, @leper wrote:

sanity of the js code

The attack test is replaced by the proposed deepfreeze exceptions.
The garrison holder test is no loss to me.
The death damage test is replaced with an equivalent test.

The removal of the clone in DeathDamage.js is correct, because the only use of the template variable refered to as splashBonus is in Damage.prototype.CauseSplashDamage at damageMultiplier *= GetDamageBonus(ent, data.splashBonus);.
The removal of the clone in Attack.js is correct, because the only use of that template variable refered to as bonus is in Damage.prototype.MissileHit, passed to GetDamageBonus in helpers/DamageBonus.js which only multiplies that number to let attackBonus.
(If I missed something, it's likely that gameboy points out the gap)

Move new test to a new file and show the file+linenumber of failed tests.

Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/tests/setup_test.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/tests/setup_test.js
|   1|   1| Engine.RegisterInterface("TestSetup");
|   2|   2| 
|   3|    |-function TestSetup() {};
|    |   3|+function TestSetup() {}
|   4|   4| TestSetup.prototype.Init = function() {};
|   5|   5| 
|   6|   6| Engine.RegisterSystemComponentType(IID_TestSetup, "TestSetup", TestSetup);

binaries/data/mods/public/simulation/components/tests/setup_test.js
|  14| »   }·catch·(e)·{}
|    | [NORMAL] ESLintBear (no-empty):
|    | Empty block statement.

binaries/data/mods/public/simulation/components/tests/setup_test.js
|   3| function·TestSetup()·{};
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  66| »   »   »   "GetOwner":·()·=>·player
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  70| »   »   »   "GetOwner":·()·=>·enemyPlayer
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  74| »   »   »   "GetOwner":·()·=>·friendlyPlayer
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  81| »   »   "GetPosition":·()·=>·new·Vector3D(4,·3,·25),
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  82| »   »   "GetRotation":·()·=>·new·Vector3D(4,·0,·6),
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js
|  83| »   »   "GetTurretParent":·()·=>·INVALID_ENTITY,
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/components/Attack.js
| 202| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 242| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 536| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 588| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/tests/test_Attack.js
| 167| »   attackComponentTest(className,·true,·(attacker,·cmpAttack,·defender)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/499/ for more details.

While still discussing the JS native functions uses, can we merge deepcopy and clone D870?

In D871#34320, @elexis wrote:

In D871#34238, @leper wrote something equal to:

should we also complain the exception message in the test?

There is no exception message, since there is no exception in case the test fails.
If we want to make it easier to debug (in particular giving the line number of the actual test that failed, I'd offer an Engine.TS_FAIL("Missed exception at " + new Error().stack);, resulting in nearly what we intend to print:

I meant checking if the exception that is thrown and that we are catching is indeed the one we were expecting. Not that last update. Could also do without that, since that is unlikely to ever catch anything than better error messages.

In D871#34325, @elexis wrote:

While still discussing the JS native functions uses, can we merge deepcopy and clone D870?

Noticed the last comment there?

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1984/ for more details.

Harbormaster completed remote builds in B3043: Diff 3544.Sep 6 2017, 9:38 PM

In D871#34333, @leper wrote:

I meant checking if the exception that is thrown and that we are catching is indeed the one we were expecting. Not that last update. Could also do without that, since that is unlikely to ever catch anything than better error messages.

Ah that.
In general a false positive (which is noticed when doing an SM upgrade and easy to fix) is preferable over a false negative (as in having an unrelated exception being unintentioanlly suprressed, thus not noticing two bugs).
in this case I don't see what other exception could ever occur (but who said that we can predict the future of JS design).
If we want to do the check, it could only compare against a human-readable string: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypeError#Properties_2
meh

In D871#34325, @elexis wrote:

While still discussing the JS native functions uses, can we merge deepcopy and clone D870?

Noticed the last comment there?

Oh, didn't. Thx.

Thanks for the review! When I can sleep, I can sleep much better knowing that this mystery is solved!

Closed by commit rP20134: Mark simulation component template and entity properties as read-only in… (authored by elexis). · Explain WhySep 8 2017, 6:23 AM

This revision was automatically updated to reflect the committed changes.

elexis mentioned this in rP20989: Rename expectException from rP20134 to TS_ASSERT_EXCEPTION..Jan 23 2018, 9:01 PM