Following D117
Details
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
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.
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? |
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. |
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.