Details
- Reviewers
fatherbushido - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19686: Add a test for the Upgrade Component, focusing on tech modifications.
Run the test. It should pass without error.
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 (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/1252/ for more details.
(That test indeed doesn't fit in any other file. I don't know what's our policy about that.
Even if I tend to prefer to test things individually, that one makes sense as it shows a bug we encountered.)
Some quick inline remarks.
Finally, after my inline comments, you can just makes that unit tests for ugrade component alone.
You can even mock TechnologyManager (mock its ApplyModification functions) and add that mock at L128.
| binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js | ||
|---|---|---|
| 28 ↗ | (On Diff #2025) | imo mock that |
| 29 ↗ | (On Diff #2025) | imo mock that |
| 80 ↗ | (On Diff #2025) | so just mock here |
| 85 ↗ | (On Diff #2025) | mock that too |
| 95 ↗ | (On Diff #2025) | all that part would be away |
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!
http://jw:8080/job/phabricator/1329/ for more details.
Build is green
Executing section Default... Executing section Source... Executing section JS... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/22/ for more details.
Build is green
Executing section Default... Executing section Source... Executing section JS... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/25/ 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!
http://jw:8080/job/phabricator/1333/ for more details.
I don't want to be annoying but perhaps you can look the comments I just add.
(You can perhaps also rename the file in test_Upgrade.js (even if we don't test all that component) - no clue about that. I will agree with your choice.)
Also nothing important but you can remove the () for single argument in arrow function if you want.
| binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js | ||
|---|---|---|
| 52 ↗ | (On Diff #2160) | I like! |
| 76 ↗ | (On Diff #2160) | so you could nuke all that |
| 85 ↗ | (On Diff #2160) | And perhaps we could so get rid of that. |
| 154 ↗ | (On Diff #2160) | I would just add a mock for the "ApplyModifications" method here (so no need to boolean...) |
Remove unnecessary code.
Also add comments mentioning where each of the Mocked components' functions are used.
Hopefully the additional line comments help make it easier to tell what's used where.
[...] you can remove the () for single argument in arrow function if you want.
Forgot to do that this time round. I'll do it for the next diff.
Build is green
Executing section Default... Executing section Source... Executing section JS... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/37/ 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!
http://jw:8080/job/phabricator/1346/ for more details.
Last comments, nothing really mandatory.
Ah yes I read to quickly to see the whole test, indeed we need the template mod. Now test is imo more isolated and test that whole past issue.
Don't know about the name of the file.
Some could say there are two many comments, but it's a matter of style and I m fine with that ;-)
Remains that partenthesis that you can remove, that newline and perhaps the missing spaces in objects in the TS_ASSERT things.
| binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js | ||
|---|---|---|
| 112 ↗ | (On Diff #2174) | ok you did the choice to do that and then call cmpPlayer.GetPlayerID() on what follow |
| 113 ↗ | (On Diff #2174) | no need I think |
| 125 ↗ | (On Diff #2174) | perhaps extra newline |
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!
http://jw:8080/job/phabricator/1362/ for more details.
Build is green
Executing section Default... Executing section Source... Executing section JS... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/49/ for more details.
Don't forget
svn propset svn:eol-style native binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js
before commiting