Page MenuHomeWildfire Games

Unit test for VisionSharing component.
ClosedPublic

Authored by fatherbushido on Feb 27 2017, 7:48 PM.

Details

Summary

Following D117

Test Plan

-

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

fatherbushido created this revision.Feb 27 2017, 7:48 PM

(Fix a typo in a comment)

Vulcan added a subscriber: Vulcan.Feb 27 2017, 11:24 PM

Build is green

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

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

Build is green

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

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

(Do we need also acceptation for tests?)

elexis added a subscriber: elexis.Mar 1 2017, 11:10 AM

Looks good. Besides OnOwnershipChanged and OnGarrisonedUnitsChanged, all functions in the component are tested, which should be sufficient for a test.
Don't forget the eol-style native attribute when committing. I can post an acceptance if you want and if the reviewers don't have any (further) comments.

binaries/data/mods/public/simulation/components/tests/test_VisionSharing.js
83 ↗(On Diff #629)

(In most occurances when functions are assigned to a variable, I see the opening brace on the same line as the function declaration, but it's not settled officially and probably inconsistent with the convention even)

134 ↗(On Diff #629)

Nice trick to check Maps

156 ↗(On Diff #629)

Where's that 48 coming from? Arbitrary right? Then we could remove it too?

fatherbushido added inline comments.Mar 1 2017, 9:19 PM
binaries/data/mods/public/simulation/components/tests/test_VisionSharing.js
156 ↗(On Diff #629)

It's arbitrary. The one in the mock above. I just choose a signifactive number to know what it is :/ And I was lazy to use a variable.

native eol-style

Build is green

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

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

Itms accepted this revision.Mar 2 2017, 9:45 AM
Itms awarded a token.

This looks good! Thanks for writing it 😃

This revision is now accepted and ready to land.Mar 2 2017, 9:46 AM
This revision was automatically updated to reflect the committed changes.