Garrison Holder has some code style issues, for example bad linebreaks, uses deprecated for each, variables out of scope, redeclared variables and the like that ought to be fixed. Most of them can be found using jshint.
Details
Details
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 2491 Build 4200: Vulcan Build Jenkins
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 454 | Missing comment for forced | |
| 484 | Looking at rP14049, it seems, like this was a bugfix, so I don't think that should be removed. | |
| 490 | Sure to remove the floor? | |
| 549–550 | I guess this should be || | |
| 661 | Why was that check added? | |
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 144 | Well it returns an array made of the values, so I don't see why no. | |
| 168 | Caused trouble when testing for different player garisonning and ungarrisonning look at the tests | |
| 425 | What is ? | |
| 661 | To protect from removing entites that are not in the garrisonholder | |
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 144 | Let's assume our Map contains of {"id1": allow1, "id2": allow2} | |
| 425 | The order the parameters appear in the functions head: function(template, owner, all, forced) | |
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 490 | parentheses around +this.template.EjectHealth needed? | |
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 170–172 | entity -> ent | |
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 578 | Don't remove this newline | |
Comment Actions
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://jw:8080/job/phabricator/1622/ for more details.
Comment Actions
(Maybe commit the GarrisonHolder.js changes before we have to embarass Stan again with requiring another update. Should still learn how to write simulation tests, it's not hard at all. Just check that every file is included that is needed (i.e. removing one causes errors. Sometimes components can be mocked instead of loading another file) and no more inclusions than needed. With AddMock you can create a component of an entity by specifying an arbitrary template manually, so that we don't have to load any other component code but solely test the code of the garrison holder here. ResetState nukes everything that was added in the test. Then with the asserts you can verify that a function of the garrison holder produces the expected result when running it on the given entity. -> TS_ASSERT_UNEVAL_(cmpReviewer.GetExcuses(), [];)
Comment Actions
(Just ask on irc if you're having questions on the tests, fatherbushido might help you too when he's online)
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 269 | JsDoc misses first parameter. | |
Comment Actions
Remove useless doxygen comments on fatherbushido's request. Add a doxygen comment for entity.
Comment Actions
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://jw:8080/job/phabricator/1679/ for more details.
Comment Actions
Thank you for your effort, your patience and the multiple rebasing...
(And sorry for forgetting to add Patch by Stan in the commit message :/)
Comment Actions
Among the list of methods (AllowGarrisoning CanPickup Eject EjectOrKill Garrison GetAllowedClasses GetCapacity GetEntities GetGarrisonedEntitiesCount GetHealRate GetLoadingRange HasEnoughHealth HealTimeout IsAllowedToGarrison IsEjectable IsFull IsGarrisoningAllowed OnDestroy OnDiplomacyChanged OnGlobalEntityRenamed OnGlobalInitGame OnGlobalOwnershipChanged OnHealthChanged OnValueModification OrderWalkToRallyPoint PerformEject PerformGarrison Unload UnloadAll UnloadAllByOwner UnloadTemplate UpdateGarrisonFlag)
some are not straightly (or not at all tested).
For example, the regeneration of health logic (we had bugs about that in a21) or the aura are not tested at all. That's not a problem, tests could be completed later (we must only have in mind that we don't have a 100% covering of that component).
One could have also try to catch some bugs (for example testing more deeply UnloadTemplate) or to test weird case but we'll perhaps also lose readability.
It's really hard to have a good (and usable) structure for that kind of tests (testing each isolated function - I am perhaps more for that, testing specific things, testing the whole thing, ...).
So to sum up:
- I am not really convinced by the structure of the tests (I am also not really convinced by the structure of some I wrote).
- They are not extensive and many logics are not tested. So specific things should be added later.
- It's better than nothing so I can commit it (after removing the wrong Player mocks and the useless ones) or we can just forget it and restart it in a way or another (on bug demand or in adding unit testing of the several methods).
EDIT: in any case, we should imo close that Diff and open a new one if needed.
| binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js | ||
|---|---|---|
| 42 | useless mock | |
| 46 | Useless if you use it with 0 value (it's the default in the code). | |
| 76 | Better returning a positive int as in the mocked function. | |
| 81 | Useless mock. If you add it, there should be a use, a meaning or something. Else there is something wrong. | |
| 84 | (no () needed) | |
| 101 | That Player mock seems a mistake. | |
| 111 | That Player mock seems a mistake. | |
| 121 | That Player mock seems a mistake. | |
| 132 | That mock is useless. | |
| 145 | useless mock | |
| 150 | Instead of overwritting, we could just add an if in the above for loop, but it's ok like that | |
| 175 | (perhaps we can precise here the meaning of 3 and 5 - it's clear in the code - but here it looks like a player id or something like that, but that's a detail). | |
Comment Actions
| binaries/data/mods/public/simulation/components/tests/test_GarrisonHolder.js | ||
|---|---|---|
| 8 | Heal not needed | |
| 23 | Not sure why we have this classes const if we dont use it everywhere. nuking. | |
| 42 | correct | |
| 46 | nuked | |
| 63 | enemyPlayer, IID_Player mock not used. nuking. (even if we liked it being complete, it shouldn't pretend to be used) | |
| 72 | garrisonHolderId, IID_Position mock not used, same procedure. | |
| 76 | Yes, expected to return N on Nth call. Only called once, so a const works for now. | |
| 81 | correct, can be removed | |
| 121 | Good catch, the entities shouldn't have the Player component. Tests don't complain about temoving the 3 mocks. | |
| 132 | Correct, removed it. | |
| 145 | This one seems to be used. At least it errors out if I remove the position one above or the identity one below, or any of the properties. | |
| 175 | 3 and 5 are misleading. | |
| binaries/data/mods/public/simulation/components/GarrisonHolder.js | ||
|---|---|---|
| 490 | They were here at the beginning so I don't know, but technically we shouldn't even have to use the + in front of it. | |