Page MenuHomeWildfire Games

Add mod documentation, specification, validation and validation test
ClosedPublic

Authored by elexis on Dec 1 2017, 7:11 PM.

Details

Summary

Since the introduction of mod support, mod properties were never well defined, nor tested for being well-defined.
An important missing part was that mod names may not contain =, <, >, that would break mod comparison tests.

This diff assumes dropping of the unneeded - and _ in mod versions and does not validate the mod "type" property D1082.

Test Plan

Opening the page is sufficient to run the first GUI test in gui/.

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

elexis created this revision.Dec 1 2017, 7:11 PM
Vulcan added a subscriber: Vulcan.Dec 1 2017, 7:15 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Dec 1 2017, 7:15 PM
Executing section Default...
Executing section Source...
Executing section JS...
elexis added a comment.Dec 1 2017, 7:27 PM

Mods already have it wrong, so mod developers should fix their names if they don't match the specs.

elexis added a comment.Dec 2 2017, 1:39 PM

Mods should likely be required to pass the scheme part of the URL (http://), see D1101 (public mod doesn't have it and Engine.OpenURL expects it too).

Itms requested changes to this revision.Dec 3 2017, 3:23 PM
Itms added a reviewer: Itms.
Itms added a subscriber: Itms.

Yay GUI tests! ?

I am very happy to see some, but I would rather have a extensible system that is used by the automated tests, not upon use by players (it might slow things down). A first thing to look into might be to create a tests/ folder somewhere (either mods/mod/gui/tests if you consider those tests the basic tests for the mod handling functionality in general, or mods/mod/gui/modmod/tests if you consider those tests strongly related to the mod selection GUI page itself - I'd say the former). I should have another try at D151 while we're at it.
Do you see a way to make this?

Itms requested changes to this revision.Dec 3 2017, 3:24 PM
This revision now requires changes to proceed.Dec 3 2017, 3:24 PM
elexis requested review of this revision.Dec 3 2017, 3:26 PM

I might, but not in this diff.

elexis added a comment.Dec 3 2017, 3:28 PM

These tests are executed when opening the GUI page. I also plan on writing rmgen/ tests. But implementing these is entirely out of scope of this diff. This diff should solely be concerned with ensuring that mods are well deined and that the regexes that test that do work as intended too.

Making this file load from the command line once we have a test platform will be very simple

Itms added a comment.Dec 3 2017, 3:31 PM

In that case I'd like this diff to be reduced to documentation and validation because I don't feel like it's a good idea to run tests each time the user uses the mod selector. Validating the actual mods the user has is already nice. Can you create a Must Have ticket for not losing your test file?

elexis added a comment.Dec 3 2017, 3:34 PM

Running the test consumes some microseconds on init and it will inform developers that might break the mod page while working on it if they break things. Since its in a separate file it will also be very easy to move somewhere else.

elexis added a subscriber: leper.
elexis added inline comments.
binaries/data/mods/mod/gui/modmod/validatemod.js
46 ↗(On Diff #4476)
In D931#38404, @leper wrote:

so just update the docs, and possibly add a check to the mod manager that this only consists of A-Za-z_0-9 and call it done.

Vulcan added a comment.Dec 9 2017, 1:33 AM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Dec 9 2017, 1:34 AM
Executing section Default...
Executing section Source...
Executing section JS...
Itms accepted this revision.Dec 10 2017, 3:40 PM

I'm ok with this patch, with the condition that you reference ticket #4427 in the commit message. We really need to stop running those tests on runtime ASAP. Thanks for addressing leper's remarks!

binaries/data/mods/mod/gui/modmod/validatemod_test.js
15 ↗(On Diff #4662)

I don't know whether that is relevant, but it should be wildfiregames.com/forum

This revision is now accepted and ready to land.Dec 10 2017, 3:40 PM
Itms updated the Trac tickets for this revision.Dec 10 2017, 3:40 PM

Thanks for the review Itms!

The now defined and asserted properties also reveal some issues in mods:

WARNING: mod name of delenda_est may only contain alphanumeric characters, but found 'Delenda Est'!

no regrets

WARNING: mod name of vox_populi_22.1.3 may only contain alphanumeric characters, but found 'Vox Populi'!

neither

WARNING: mod name of vox_populi_22.1.3 may only contain numbers and at most 2 periods, but found '0.0.22.1.3'!

This mod version was broken before, so now it's revealed.

binaries/data/mods/mod/gui/modmod/modmod.js
29 ↗(On Diff #4662)

The regex also allows underscore and dash.

52 ↗(On Diff #4662)

Forgot to delete these two when rebasing

binaries/data/mods/mod/gui/modmod/validatemod_test.js
15 ↗(On Diff #4662)

thx

This revision was automatically updated to reflect the committed changes.