Page MenuHomeWildfire Games

Support RandomMap tests
ClosedPublic

Authored by elexis on Jul 16 2019, 5:10 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23455: Support random map script tests, fixes #4827.
Trac Tickets
#4827
Summary

pyrogenesis/0ad uses the CxxTest framework to support testing of functions.
Currently only the engine and simulation is covered by that, missing are random map generation scripts and gui scripts.
This implements the backend for map generation testing.

Test Plan

Includes D2084, so that should be reviewed independently.
Consider whether the m_ScriptInterface is deleted in testmode in the c++ test file as it is not allocated with new.
Consider whether the C++ engine provides all functions needed to the ScriptInterface (for example Xeromyces is needed for the TemplateLoader functions registered in RegisterScriptFunctions of the MapGenerator),
compare with the simulation script test code simulation2/components/tests/test_scripts.h.

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.Jul 16 2019, 5:10 PM

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

Linter detected issues:
Executing section Source...

source/graphics/MapGenerator.h
|  40| class·CMapGenerator
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCMapGenerator{' is invalid C code. Use --std or --language to configure the language.

source/graphics/tests/test_MapGenerator.h
|  23| class·TestMapGenerator·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestMapGenerator:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|  10|  10| 			this.prefix +
|  11|  11| 			"Generating " + g_MapSettings.Name +
|  12|  12| 			" of size " + g_MapSettings.Size +
|  13|    |-			" and "  + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|    |  13|+			" and " + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|  14|  14| }
|  15|  15| 
|  16|  16| RandomMapLogger.prototype.printDirectly = function(string)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|  59|  59| 	TS_ASSERT(!tileClass.has(point));
|  60|  60| }
|  61|  61| 
|  62|    |-//Test Multi-insertion removal
|    |  62|+// Test Multi-insertion removal
|  63|  63| {
|  64|  64| 	let tileClass = new TileClass(55);
|  65|  65| 	let point = new Vector2D(5, 4);
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/96/display/redirect

wraitii added a subscriber: wraitii.

It seems to me like you are running 'integration' tests here, but your test is actually a unit test.
I would prefer if unit tests remained unit tests and didn't require the map generator to actually work (see how component / helpers tests do it - it should just be one line to add that). This would be faster and less code, obviously.

If you want integration tests, instead you should run the map generation and then run scripts to check the output of that.

wraitii added reviewers: wraitii, Restricted Owners Package.Jul 17 2019, 10:01 AM

You mean it's an integration test because the dependent JS and C++ code is also tested rather than being mocked? There is no map generated.
Actually the simulation tests also load other JS code instead of mocking it, see 45 occurrences of LoadHelperScript, which is just the same as the load file function registered here.
Don't let the name mislead you, MapGenerator is just the JS Interface, MapReader is the one that otherwise starts the MapGenerator ad handles the processing of the result of the scripts. There is no result ever passed here, nor is the result parsing code called here.

it should just be one line to add that

I don't understand what you mean, especially what line.
In case you want me to look at how simulation component script tests are implemented, that's how I started this patch.

binaries/data/mods/public/maps/random/tests/test_TileClass.js
1 ↗(On Diff #8930)

I have a patch to load individual files, but it looks like it's a case of "everything needs everything else". To be reviewed separately.

Mh, I did misread that somewhat, sorry.

You mean it's an integration test because the dependent JS and C++ code is also tested rather than being mocked?

Well you are using the map generator code, instead of just loading the scripts.

Actually the simulation tests also load other JS code instead of mocking it, see 45 occurrences of LoadHelperScript, which is just the same as the load file function registered here.

That's not what I was complaining about, indeed https://github.com/0ad/0ad/blob/master/source/simulation2/components/tests/test_scripts.h does register functions.

Don't let the name mislead you, MapGenerator is just the JS Interface, MapReader is the one that otherwise starts the MapGenerator ad handles the processing of the result of the scripts. There is no result ever passed here, nor is the result parsing code called here.

Indeed, however notice that https://github.com/0ad/0ad/blob/master/source/simulation2/components/tests/test_scripts.h explicitly loads the tests scripts. You don't have to go through the map generator worker at all here.

it should just be one line to add that

I don't understand what you mean, especially what line.
In case you want me to look at how simulation component script tests are implemented, that's how I started this patch.

Yeah actually I forgot that this file is explicitly about component scripts. In D1846 I just added a new function there for global scripts.
I think the best way to do this would be to move source/simulation2/components/tests/test_scripts.h to the script interface tests, and just use it for all JS unit-tests, since they all require the same kind of setup.
Integration tests would be more involved, so those would be in a specific location.

Well you are using the map generator code, instead of just loading the scripts.

So we rename the MapGenerator to JSInterface and it's done? Because that's just a JS interface that can run scripts that can be maps or map tests.

I don't think you need to use the map worker's script interface to unit test map script stuff though.

Then one needs to duplicate LoadScripts, TERRAIN_TILE_SIZE, MAP_BORDER_WIDTH, the other constants to be added there, still needs LoadGlobalScripts to support Vectors etc., needs to replicate m_LoadedLibraries behavior unless one wants to drop that. I suppose LoadMapTerrain and LoadHeightmap never will receive tests. SetCallbackData, ReplaceNondeterministicRNG also needed I guess.
If we want mapgen scripts to support loading invidiual files instead of directories, that would also be copied.
So mostly it seems to me that the MapGenerator functions need to be duplicated if they're not included.
One can subscribe less functions, for example can remove ExportMap and SetProgress and template loader functions for test cases.
I guess the constants TERRAIN_TILE_SIZE can be duplicated / mocked in every scriptfile as well.

Indeed, however notice that https://github.com/0ad/0ad/blob/master/source/simulation2/components/tests/test_scripts.h explicitly loads the tests scripts. You don't have to go through the map generator worker at all here.

MapGenerator is for this test what the CComponentManager is in that other test.

I will check.

wraitii added a comment.EditedJul 17 2019, 1:20 PM

MapGenerator is for this test what the CComponentManager is in that other test.

Ah, right.
But as you can see, the component manager is only loading the component types, it's not loading the functions such as "AddEntity". Indeed, that gets mocked in https://github.com/0ad/0ad/blob/master/binaries/data/mods/public/simulation/components/tests/setup.js
I think calling "Initialize" and "Run" is rather ugly - in comparison then component manager only loads component types.

Then one needs to duplicate LoadScripts, TERRAIN_TILE_SIZE, MAP_BORDER_WIDTH, the other constants to be added there, still needs LoadGlobalScripts to support Vectors etc., needs to replicate m_LoadedLibraries behavior unless one wants to drop that. I suppose LoadMapTerrain and LoadHeightmap never will receive tests. SetCallbackData, ReplaceNondeterministicRNG also needed I guess.

Well, the RNG seed needs to be deterministic for tests. The RM settings aren't needed for unit Tests.
As for the rest, we should do like in component tests. AKA you set LoadLibrary, LoadHeightmapImage, LoadMapTerrain manually, and the rest you load in a JS script so that tests scripts can overload them if necessary. For example it's likely that a tests would want to mock GetTemplate, which is doable if it's not set by the C++ engine.

So mostly it seems to me that the MapGenerator functions need to be duplicated if they're not included.

Indeed, but that's not much of an issue, it's only a few lines.
EDIT: welp, LoadScripts is rather big. SO maybe we need a way to load the actual function instead of a mock there.

Edit2: perhaps the cleanest would be to split the initialisation that we need for tests from the rest and just call that.

elexis added a comment.EditedJul 22 2019, 12:47 PM

I will check.

So I did, I upload the result as a file attachment, because I'm not fully convinced that it will be better:

Now it looks nice at first glance, because the entire dependency is avoided, but that's also it's greatest disadvantage.

1. Duplicate script loading:
The first disadvantage is that the LoadScripts function is duplicated.
I didn't transfer the check for duplicate calls and ended up compiling and debugging the C++ code for half an hour, looking for the hidden secret as to why rmgen/ may be loaded twice instead of the expected once justifying that code, until I noticed there was an unrevisioned file that had its contents duplicated thus calling include rmgen twice.
So it seems that redundancy check might not necessary in MapGenerator if the JS inclusion is clean. But perhaps it would still be better to throw a JS error stack on redundant scriptloads instead of silently ignoring it.
The GUI script loading analogon CGUI::Xeromyces_ReadScript actually also keeps track of loaded scripts, avoids duplicate directory inclusions but does not avoid duplicate file inclusions, so it's (finding a bug while)^N+finding a bug that need to be fixed independently.
Perhaps one can do a common scriptloader class to make it consistent in gui, mapgen, mapgentests.

2. Duplicate engine constants:
The idea of #4034 was to remove and reduce duplication of engine constants, but if not letting the MapGenerator define the constants, then there will be more duplication again, which may be admissible but also not so nice IMO as just letting the MapGenerator define commonly.

Another issue is that Engine.GetMicroseconds is required as the RandomMap is registers the RandomMapLogger using it.
This dependency relationship should probably be broken by letting every map create its own logger; even though it will mean that some maps hypothetical maps may introduce weird unjustifiably inconsistent duplicate stuff.
Edit: Actually the call in RandomMap.prototype.exportEntityList and RandomMap.prototype.ExportMap contribute as to why thats part of RandomMap.

3. PMP/PNG terrain loading:
I suppoes the two terrain loading functions would be tested by providing small testspecific files and seeing that they return expected data and fail appropriately otherwise.
It seems inviting to perform this test in JS instead of C++.

Template loader functions seem worthy to mock, which relativizes the above disadvantages of dropping the dependency.

4. Duplicate future mapgen registrations:
There are more functions, globals and classes exposed in the MapGenerators ScriptInterface in the future, so that will also mean keeping the test scriptinterface registration duplication in sync instead of calling an existing function subscribing those.

Edit2: perhaps the cleanest would be to split the initialisation that we need for tests from the rest and just call that.

That's what this patch does, except for throwing out the template loader and progress functions.

Perhaps one could move the shared part to a new file, but this also means it would be better to relocate this out of graphics/ instead of adding another incorrectly mapgenerator file there.
But then it's also questionable and dubious as to whether it makes sense having this bit of common code in a separate file (no identity, hard to name, mapgenHelper? meh).

So it seems like a battle of unwanted duplication vs unwanted dependencies. To me the duplication seems to slightly outweigh the loading of a c++ class that has some functions that will not be used in that context rather than copying the functions that are used in both places, even if only little code and even if some of that can be duplicated in JS instead of C++.

source/graphics/tests/test_MapGenerator.h
30 ↗(On Diff #8930)

(See this undesirable public/? Can only happen with this mod layout, refs ticket)

elexis updated this revision to Diff 9051.Jul 22 2019, 12:50 PM

Upload the version without MapGenerator inclusion for demonstration purposes, don't like it.

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

Linter detected issues:
Executing section Source...

source/graphics/tests/test_MapGenerator.h
|  23| class·TestMapGenerator·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestMapGenerator:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|  10|  10| 			this.prefix +
|  11|  11| 			"Generating " + g_MapSettings.Name +
|  12|  12| 			" of size " + g_MapSettings.Size +
|  13|    |-			" and "  + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|    |  13|+			" and " + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|  14|  14| }
|  15|  15| 
|  16|  16| RandomMapLogger.prototype.printDirectly = function(string)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|  60|  60| 	TS_ASSERT(!tileClass.has(point));
|  61|  61| }
|  62|  62| 
|  63|    |-//Test Multi-insertion removal
|    |  63|+// Test Multi-insertion removal
|  64|  64| {
|  65|  65| 	let tileClass = new TileClass(55);
|  66|  66| 	let point = new Vector2D(5, 4);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 392| 392| 				// Players see colors depending on diplomacy
| 393| 393| 				g_DisplayedPlayerColors[i] =
| 394| 394| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 395|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 395|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 396| 396| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 397| 397| 					getDiplomacyColor("enemy");
| 398| 398| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 393| 393| 				g_DisplayedPlayerColors[i] =
| 394| 394| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 395| 395| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 396|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 396|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 397| 397| 					getDiplomacyColor("enemy");
| 398| 398| 
| 399| 399| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 394| 394| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 395| 395| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 396| 396| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 397|    |-					getDiplomacyColor("enemy");
|    | 397|+								getDiplomacyColor("enemy");
| 398| 398| 
| 399| 399| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 400| 400| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 644| 644| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
| 645| 645| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
| 646| 646| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
| 647|    |-			});
|    | 647|+				});
| 648| 648| 	}
| 649| 649| 
| 650| 650| 	// Following gaia can be interesting on scripted maps
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1194|1194| 
|1195|1195| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1196|1196| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1197|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1197|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1198|1198| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1199|1199| 	});
|1200|1200| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1195|1195| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1196|1196| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1197|1197| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1198|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1198|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1199|1199| 	});
|1200|1200| 
|1201|1201| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1196|1196| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1197|1197| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1198|1198| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1199|    |-	});
|    |1199|+		});
|1200|1200| 
|1201|1201| 	let resCodes = g_ResourceData.GetCodes();
|1202|1202| 	for (let r = 0; r < resCodes.length; ++r)

binaries/data/mods/public/gui/session/session.js
|1055| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1130| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1131| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1132| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/182/display/redirect

I guess I'll agree with the diff you like when you upload that one.

2. Duplicate engine constants:
The idea of #4034 was to remove and reduce duplication of engine constants, but if not letting the MapGenerator define the constants, then there will be more duplication again, which may be admissible but also not so nice IMO as just letting the MapGenerator define commonly.

The question is "do we ever want to mock engine constants for testing?" If yes, they should be duplicated for tests - I don't think this is too outlandish. If not, we can just load the core ones.
I guess in most cases map tests won't want to change engine constants so it's probably fine.

Things like map settings however would be nice to mock in the tests, so we can change map size, ...


Basically my point is that what tests would find useful to change should be duplicated for tests, the rest can stay. I don't like going through "Run" and "Init" still.

elexis updated this revision to Diff 9057.Jul 22 2019, 2:01 PM

I hate this code.

elexis updated this revision to Diff 9058.Jul 22 2019, 2:02 PM

I hate this code + missing file.

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

Linter detected issues:
Executing section Source...

source/graphics/MapGenerator.h
|  40| class·CMapGenerator
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCMapGenerator{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|  61|  61| 	TS_ASSERT(!tileClass.has(point));
|  62|  62| }
|  63|  63| 
|  64|    |-//Test Multi-insertion removal
|    |  64|+// Test Multi-insertion removal
|  65|  65| {
|  66|  66| 	let tileClass = new TileClass(55);
|  67|  67| 	let point = new Vector2D(5, 4);
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|  10|  10| 			this.prefix +
|  11|  11| 			"Generating " + g_MapSettings.Name +
|  12|  12| 			" of size " + g_MapSettings.Size +
|  13|    |-			" and "  + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|    |  13|+			" and " + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|  14|  14| }
|  15|  15| 
|  16|  16| RandomMapLogger.prototype.printDirectly = function(string)
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/187/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/188/display/redirect

elexis updated this revision to Diff 9059.Jul 22 2019, 2:05 PM

All files this time, and I still unsatisfied with the code.

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

Linter detected issues:
Executing section Source...

source/graphics/MapGenerator.h
|  40| class·CMapGenerator
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCMapGenerator{' is invalid C code. Use --std or --language to configure the language.

source/graphics/tests/test_MapGenerator.h
|  23| class·TestMapGenerator·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestMapGenerator:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|  61|  61| 	TS_ASSERT(!tileClass.has(point));
|  62|  62| }
|  63|  63| 
|  64|    |-//Test Multi-insertion removal
|    |  64|+// Test Multi-insertion removal
|  65|  65| {
|  66|  66| 	let tileClass = new TileClass(55);
|  67|  67| 	let point = new Vector2D(5, 4);
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|  10|  10| 			this.prefix +
|  11|  11| 			"Generating " + g_MapSettings.Name +
|  12|  12| 			" of size " + g_MapSettings.Size +
|  13|    |-			" and "  + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|    |  13|+			" and " + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|  14|  14| }
|  15|  15| 
|  16|  16| RandomMapLogger.prototype.printDirectly = function(string)
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/189/display/redirect

I think you're still embarking too much. The tests should be able to set up its own non-threaded environment by calling a function, and the actual code should call a "startThread" function calling that same function.

You might need to make the test class a friend of the map generator to call private members directly, but I think that's fine overall.

I think you're still embarking too much.

Too much what?

The tests should be able to set up its own non-threaded environment by calling a function

That's the case.

and the actual code should call a "startThread" function calling that same function.

The code in this diff is actual, I guess you mean the map generation code, and that is using a "runThread" function.

Either the mapgen code is using this class or its duplicating it.

You might need to make the test class a friend of the map generator to call private members directly, but I think that's fine overall.

But that's not reducing code, that's adding to it, and the class still has to be instantiated since the scriptfunctions access its private members, such as the scriptfiles loaded etc.
That's either duplicated in C++ or duplicated in JS or feature dropped and later introduced again when the feature becomes necessary again, or not duplicated but reused.

See inline - I don't think you need to add testing-specific code in MapGenerator at all with some very small duplication in tests, which is perfectly fine imo.

source/graphics/MapGenerator.cpp
71 ↗(On Diff #9059)

Use std::thread and m_WorkerThread.joinable() instead, see https://code.wildfiregames.com/D1917#change-h8KW8LIyVdrv

90 ↗(On Diff #9059)

Why call this at all during testing?

142 ↗(On Diff #9059)

I would rather you directly called RegisterScriptFunctions_Common() in test_mapgen_scripts, set m_MapGenRNG to 0, and called m_ScriptInterface->LoadGlobalScriptFile(path);, and just let this be the old Run() function for the actual game code.

Then you no longer need m_Testing or calling Initialize()

elexis updated this revision to Diff 11205.Jan 28 2020, 2:11 PM

Use the approach where the test class duplicates some of the logic rather than calling into the mapgenerator.
Minimize that duplication by performing the SetCallbackData call, the ReplaceDeterministicRNG call and the Seed(0) call in the MapGeneratorWorker constructor.

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

Linter detected issues:
Executing section Source...

source/graphics/MapGenerator.h
|  41| class·CMapGenerator
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCMapGenerator{' is invalid C code. Use --std or --language to configure the language.

source/graphics/tests/test_MapGenerator.h
|  23| class·TestMapGenerator·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestMapGenerator:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/RandomMapLogger.js
|  10|  10| 			this.prefix +
|  11|  11| 			"Generating " + g_MapSettings.Name +
|  12|  12| 			" of size " + g_MapSettings.Size +
|  13|    |-			" and "  + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|    |  13|+			" and " + (g_MapSettings.PlayerData.length - 1) + " players.\n");
|  14|  14| }
|  15|  15| 
|  16|  16| RandomMapLogger.prototype.printDirectly = function(string)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/tests/test_TileClass.js
|  59|  59| 	TS_ASSERT(!tileClass.has(point));
|  60|  60| }
|  61|  61| 
|  62|    |-//Test Multi-insertion removal
|    |  62|+// Test Multi-insertion removal
|  63|  63| {
|  64|  64| 	let tileClass = new TileClass(55);
|  65|  65| 	let point = new Vector2D(5, 4);
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1672/display/redirect

There are two different ways this could be implemented.

One way is that the testclass code is minimal and does not specify own logic. This was done in the previous approaches (m_Threading, or m_Testing).
This has the advantage that when changing the MapGeneratorWorker, one does not have to duplicate the testing code, nor does one have to make up for the differences between the mapgenerator and test code class setup (for instance threading vs not threading), since everything that is relevant to setup the MapGeneratorWorker would be contained in this one class.

The other way to implement it is to duplicate all of the MapGeneratorWorker code and remove the statements that are not relevant to the testcode.
This has the disadvantage that someone adding something to the MapGeneratorWorker must be cautious to insert the same statement into the testcode, or consider that the statement should not be there.
(Just because the testcode isnt broken doesnt inherently mean the statement shouldn't be there)

Thats the reason why I went for the MapGeneratorWorker::m_Testing variant before.
But implementing wraitiis proposal from last comment, and then moving the SetCallbackData, ReplaceNondeterministicRNG, and seed(0) call to a common helper function would leave the duplicative part to only 3 statements (MapGeneratorWorker ctor + 1 helper function call + LoadGlobalScript).
This way it would be somewhat easy to maintain if one isnt doing something big / new (and in that case, one can refactor the little test code as well again).

Had a brief discussion with Vladislav about this on irc today http://irclogs.wildfiregames.com/2020-01/2020-01-28-QuakeNet-%230ad-dev.log

The seed(0) is performed in this code in order to ensure the same seed for all platforms, ensuring that the test success doesn't depend on a specific platform.

From https://www.boost.org/doc/libs/1_49_0/doc/html/boost/random/rand48.html

rand48();
Seeds the generator with the default seed.

(I've made the seed an argument passed from the testcode / passed from the JS GUI so that we are sure that the seed is always initialized to a known value (refs "default seed" https://www.boost.org/doc/libs/1_49_0/doc/html/boost/random/rand48.html which makes the impression of being the same with all versions, but making it explicit is safer) and without seed being present in more than one line of code.)

This revision was not accepted when it landed; it landed in state Needs Review.Jan 29 2020, 1:30 AM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jan 29 2020, 1:30 AM