- Remove linear from DeathDamage schema (as we don't have direction). Or: One could also make in the code the direction we are looking to.
- Remove bonus from DeathDamage schema: not used and I guess meaningless
- The code for GetAttackBonus actually support splash in the Attack schema as shown in the added test. But then it's not used. Moreover what would be the meaning of such a bonus (as it concerns the aimed target not the splash damaged one). I would be for removing it (not done in the patch).
Details
- Reviewers
Mate-86 - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19985: Add some tests for Attack component. Some cleanup. Reviewed by Mate-86.
Needs an input mainly for the 4th point.
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 has FAILED
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests). In TestComponentScripts::test_scripts: ../../../source/test_setup.cpp:134: Error: Test failed: L"Stack trace:\n@simulation/components/tests/test_Attack.js:168:1\nattackComponentTest@simulation/components/tests/test_Attack.js:117:2\n@simulation/components/tests/test_Attack.js:165:2\nExpected equal, got 1 !== 2" ................................................................................................................................................................................................................................................................................................................ Failed 1 and Skipped 0 of 306 tests Success rate: 99%
Link to build: http://jw:8080/job/phabricator/1838/
See console output for more information: http://jw:8080/job/phabricator/1838/console
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Damage.js | 83| » » playersToDamage.push(i) | | [NORMAL] JSHintBear: | | Missing semicolon. binaries/data/mods/public/simulation/components/Damage.js | 86| } | | [NORMAL] JSHintBear: | | Missing semicolon. | | [NORMAL] ESLintBear (no-undef-init): | | It's not necessary to initialize 'attack' to undefined. |----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/tests/test_Attack.js | |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/tests/test_Attack.js | 229| 229| if (!isBuilding) | 230| 230| allowCapturing.push(false); | 231| 231| | 232| |- let attack = undefined; | | 232|+ let attack; | 233| 233| if (defenderClass == "Domestic") | 234| 234| attack = "Slaughter"; | 235| 235| else if (defenderClass == "Structure") binaries/data/mods/public/simulation/components/tests/test_Attack.js | 165| » attackComponentTest(className,·true,·(attacker,·cmpAttack,·defender)·=>·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. binaries/data/mods/public/simulation/components/tests/test_Attack.js | 232| » » let·attack·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'attack' to 'undefined'. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/379/ 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://jw:8080/job/phabricator/1839/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Damage.js | 86| } | | [NORMAL] JSHintBear: | | Missing semicolon. | | [NORMAL] ESLintBear (no-undef-init): | | It's not necessary to initialize 'attack' to undefined. |----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/tests/test_Attack.js | |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/tests/test_Attack.js | 229| 229| if (!isBuilding) | 230| 230| allowCapturing.push(false); | 231| 231| | 232| |- let attack = undefined; | | 232|+ let attack; | 233| 233| if (defenderClass == "Domestic") | 234| 234| attack = "Slaughter"; | 235| 235| else if (defenderClass == "Structure") binaries/data/mods/public/simulation/components/tests/test_Attack.js | 165| » attackComponentTest(className,·true,·(attacker,·cmpAttack,·defender)·=>·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. binaries/data/mods/public/simulation/components/tests/test_Attack.js | 232| » » let·attack·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'attack' to 'undefined'. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/380/ 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://jw:8080/job/phabricator/1841/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/tests/test_Attack.js | 165| » 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://jw:8080/job/phabricator_lint/382/ for more details.
- I tested the change locally by running the game. DeathDamage still works and it does not throw any errors.
- Removal of the bonusSchema and linear shape makes sense because they are not used in DeathDamage component.
- This patch is OK to submit.
Thanks for the review.
In fact, I had an idea meanwhile :)
Edit: well I will commit as such (without nuking the splash bonus part) and fix the bonus stuff in another diff. (In fact we need to parse the bonus template instead of the multiplier and move most of the getAttackBonus to Damage or even DamageReceiver and taking care for the capture part).