- 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
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
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2762 Build 4722: Vulcan Build (Windows) Jenkins Build 4721: Vulcan Build Jenkins Build 4720: arc lint + arc unit
Event Timeline
Comment Actions
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
Comment Actions
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.
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/1839/ for more details.
Comment Actions
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.
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/1841/ for more details.
Comment Actions
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.
Comment Actions
- 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.
Comment Actions
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).