Page MenuHomeWildfire Games

Fail test suite when map tests throw errors
Needs RevisionPublic

Authored by badosu on May 1 2020, 4:16 AM.

Details

Reviewers
elexis
Summary

As reported in D2710, when any javascript error is raised while running map script tests, fail the suite from rP23455. This change is necessary so we can catch any regression that is not tested via assertions that may raise exceptions.

Test Plan
  1. Change any map test to raise an exception without failing the assertions already present, e.g.:
  // binaries/data/mods/public/maps/random/tests/test_RectPlacer.js
  TS_ASSERT(area.contains(max));
  TS_ASSERT(area.contains(max.clone()));
  TS_ASSERT(area.contains(Vector2D.average([min, max])));

  boom();
}
  1. Run the tests
  2. Verify the program has returned with non-zero status

Event Timeline

badosu created this revision.May 1 2020, 4:16 AM
Owners added a subscriber: Restricted Owners Package.May 1 2020, 4:16 AM
badosu updated this revision to Diff 11754.May 1 2020, 4:18 AM

Style fixes

badosu updated this revision to Diff 11755.May 1 2020, 4:20 AM

Style fixes

badosu edited the test plan for this revision. (Show Details)May 1 2020, 4:22 AM
badosu edited the test plan for this revision. (Show Details)
badosu edited the test plan for this revision. (Show Details)
badosu edited the summary of this revision. (Show Details)May 1 2020, 4:24 AM
Vulcan added a comment.May 1 2020, 5:13 AM

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

Linter detected issues:
Executing section Source...

source/graphics/tests/test_MapGenerator.h
|  22| 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...
Executing section cli...

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

Vulcan added a comment.May 1 2020, 5:17 AM

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

Linter detected issues:
Executing section Source...

source/graphics/tests/test_MapGenerator.h
|  22| 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...
Executing section cli...

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

Vulcan added a comment.May 1 2020, 5:21 AM

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

Linter detected issues:
Executing section Source...

source/graphics/tests/test_MapGenerator.h
|  22| 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...
Executing section cli...

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

elexis edited the summary of this revision. (Show Details)May 1 2020, 11:47 AM
Stan added a subscriber: Stan.May 1 2020, 11:56 AM

Does this fail without the public mod?

elexis edited the summary of this revision. (Show Details)Mon, May 18, 11:45 PM
elexis added a subscriber: elexis.

I hereby review this patch based on the grounds that it is a defect I introduced in the feature of rP23455.

Defect and direction for resolution was discussed on 2020-04-30 in room arena23b on lobby.wildfiregames.com:

(23:37:23) badosu: we dont fail the build if javascript throws errors
(23:37:27) badosu: i want the build to fail
(23:37:37) badosu: if i cant catch errors, then ill have to make stupid tests
(23:37:56) elexis: which error for instance? out of bounds?
(23:38:03) elexis: OOB access for the heightmap?
(23:38:21) badosu: yeah, when an area tries to access points outside the grid map
(23:38:30) badosu: for example if the placer passes those points
(23:38:40) badosu: stan makes a fair comment that the placer should be tested
(23:38:51) badosu: i would prefer to catch javascript errors and fail the build
(23:39:02) badosu: but if we cant, then ill have to do placer tests decoupled from area for this
(23:39:31) elexis: if there is a JS error then the tests program will end with that error
(23:39:37) elexis: is that correct?
(23:39:44) badosu: no
(23:39:49) badosu: it will output the message
(23:39:52) badosu: but will return 0
(23:40:29) badosu: one could hackily grep the build output and ensure no 'javascript error' string is present but that is ugly af
(23:40:49) elexis: but thats out of scope of the DiskPlacer+test
(23:40:57) elexis: Running cxxtest tests (336 tests).....ERROR: JavaScript error: maps/random/tests/test_RectPlacer.js line 3
(23:41:09) badosu: yes, but it returns 0
(23:41:25) badosu: it shouldnt i guess
(23:41:31) elexis: the test code implementation is done using this one library
(23:41:37) elexis: cxxtest
(23:42:51) elexis: oh
(23:43:04) elexis: I guess thats my mistake
(23:43:21) elexis: a JS error in a simulation test will make the test fail, but not so in the rmgen code
(23:45:18) badosu: yeah, if we cant capture these erros and fail the build if they happen, ill have to change the placer tests
(23:53:49) elexis: badosu: https://code.wildfiregames.com/rP23455 introduced random map tests, ./source/simulation2/components/tests/test_scripts.h has the simulation code tests, so by comparing the two codebases one may find the change needed to make rmgen tests fail in case of JS error
(23:55:10) badosu: tks elexis gonna check it out
(00:03:05) badosu: elexis
(00:03:07) badosu: i found something
(00:03:15) badosu: https://github.com/0ad/0ad/blob/master/source/scriptinterface/ScriptInterface.cpp#L84-L122
(00:03:29) badosu: maybe i can piggyback on something used here and check if a warning/error was emitted
(00:05:42) elexis: scriptInterface.LoadGlobalScriptFile
(00:06:04) elexis: when executing a script there is usually a return value, if its false there was an error I think
(00:08:03) badosu: hmm
(00:08:07) badosu: id have to study that
(00:08:34) badosu: if i could just make a property on the errorreporter to check if any warning or error was rasied
(00:08:40) badosu: that would already be a fic
(00:08:41) badosu: fix
(00:09:00) badosu: if i can access the errorreporter on the mapgen scripts runner

Dynamic correctness test:
Without the patch applied:

Running cxxtest tests (333 tests).....ERROR: JavaScript error: maps/random/tests/test_RectPlacer.js line 6
Error: heh
  @maps/random/tests/test_RectPlacer.js:6:1
........................................................................................................................................................................................................................................................................................................................................OK!

With the patch applied:

Running cxxtest tests (333 tests).....ERROR: JavaScript error: maps/random/tests/test_RectPlacer.js line 6
Error: heh
  @maps/random/tests/test_RectPlacer.js:6:1

In TestMapGenerator::test_mapgen_scripts:
/home/elexis/code/0ad-svn5/trunk/source/graphics/tests/test_MapGenerator.h:57: Error: Assertion failed: scriptInterface.LoadGlobalScriptFile(path)

Static correctness test:
Besides working correctly at face value, it may also be broken by omission of case treatments that could systematically be discovered by comparison with existing similar code (or something like that).
As mentioned in the lobby and D2710#114485, the simulation code does catch this correctly. It uses

TSM_ASSERT(L"Running script "+pathname.string(), scriptInterface.LoadScript(pathname, content));

The difference lies in the resulting error message:

/home/elexis/code/0ad-svn5/trunk/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script globalscripts/tests/test_Resources.js"
/home/elexis/code/0ad-svn5/trunk/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)

Completeness test:
As mentioned in https://code.wildfiregames.com/D2710#114485, do the globalscripts tests from rP22970 have the same issue?
Reading the code I guess not since it uses the simulation code, which in turn makes me wonder why it does, but I guess thats not the subject of this diff. (In case of revision emergency see also lobby conversation last night)
Confirmed by test:

Error: hoh
  @globalscripts/tests/test_Resources.js:1:1

In TestComponentScripts::test_global_scripts:
/home/elexis/code/0ad-svn5/trunk/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script globalscripts/tests/test_Resources.js"
/home/elexis/code/0ad-svn5/trunk/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
.....................................................................................................
Failed 1 and Skipped 0 of 333 tests
Success rate: 99%

I fail to find other tests where the test code is located in a script file.
So the presentation of the file location of the C++ tests cant be compared directly (since there is only 1 location, not 2 as with script tests).
C++ test failures appear like this on unix:

In TestComponentManager::test_AllocateNewEntity:
/home/elexis/code/0ad-svn5/trunk/source/simulation2/tests/test_ComponentManager.h:81: Error: Expected (man.AllocateNewEntity() == (u32)4), found (2 != 4)

So it seems there is enough information provided (JS error stack + filename), but perhaps it wouldn't hurt to make the messages consistent, especially given that globalscripts and simulation ones are consistent already.
I still can commit this version too otherwise..
With your nickname in the credits, taken from D2710.

I don't boot Windows now to check if the error appears differently there. If it does, then at least it's consistent with simulation and globalscripts.
The verification frame escalated again. -.-

Stan added a comment.Tue, May 19, 10:26 AM

Without the patch (Test failure not related) @(rP23677)

PS C:\Dev\trunk> .\binaries\system\test.exe
Running cxxtest tests (340 tests).......................................................................................................................................................................................................................................................................................................
In TestSerializer::test_Debug_types:
C:\Dev\trunk\source\simulation2\tests\test_Serializer.h:145: Error: Expected (std::string(stream.str()) == std::string("# comment\n" "i8: -123\n" "u8: 255\n" "i16: -12345\n" "u16: 56789\n" "i32: -123\n" "u32: 4294967173\n" "float: 1e+30\n" "double: 1.0000000000000001e+300\n" "fixed: 1234.5\n" "t: true\n" "f: false\n" "string: \"example\"\n" "string 2: \"example\\\"\\\\\\\"\"\n" "string 3: \"example\\n\\ntest\"\n" "string 4: \"t\xC3\xAAst\"\n" "raw bytes: (6 bytes) 00 01 02 03 0f 10\n")), found ("# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n" != "# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1.0000000000000001e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n")
............................................
Failed 1 and Skipped 0 of 340 tests
Success rate: 99%

With dbgview (warnings not showing in cmd or powershell)

[29960] HRT: activating HPET failed: Unknown error (-100022, 0xFFFFFFFFFFFE794A)
[29960] HRT: using name=TSC freq=2208000000.000000
[29960] HRT: counter=TSC freq=2.208e+09 res=4.52899e-10 bits=64
[29960] ERROR: heh
[29960] 
[29960] (dumping stack frames may result in access violations...)
[29960]   locals: 1 w a 0 0 1 1
[29960] (done dumping stack frames)

With patch

PS C:\Dev\trunk> .\binaries\system\test.exe
Running cxxtest tests (340 tests).......................................................................................................................................................................................................................................................................................................
In TestSerializer::test_Debug_types:
C:\Dev\trunk\source\simulation2\tests\test_Serializer.h:145: Error: Expected (std::string(stream.str()) == std::string("# comment\n" "i8: -123\n" "u8: 255\n" "i16: -12345\n" "u16: 56789\n" "i32: -123\n" "u32: 4294967173\n" "float: 1e+30\n" "double: 1.0000000000000001e+300\n" "fixed: 1234.5\n" "t: true\n" "f: false\n" "string: \"example\"\n" "string 2: \"example\\\"\\\\\\\"\"\n" "string 3: \"example\\n\\ntest\"\n" "string 4: \"t\xC3\xAAst\"\n" "raw bytes: (6 bytes) 00 01 02 03 0f 10\n")), found ("# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n" != "# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1.0000000000000001e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n")
[29096] HRT: activating HPET failed: Unknown error (-100022, 0xFFFFFFFFFFFE794A)
[29096] HRT: using name=TSC freq=2208000000.000000
[29096] HRT: counter=TSC freq=2.208e+09 res=4.52899e-10 bits=64
[29096] ERROR: heh
[29096] 
[29096] (dumping stack frames may result in access violations...)
[29096]   locals: 1 w a 0 0 1 1
[29096] (done dumping stack frames)
Index: binaries/data/mods/public/maps/random/tests/test_RectPlacer.js
===================================================================
--- binaries/data/mods/public/maps/random/tests/test_RectPlacer.js	(revision 23677)
+++ binaries/data/mods/public/maps/random/tests/test_RectPlacer.js	(working copy)
@@ -3,6 +3,7 @@
 var g_MapSettings = { "Size": 512 };
 var g_Map = new RandomMap(0, 0, "blackness");
 
+error("heh")
 {
 	let min = new Vector2D(5, 5);
 	let max = new Vector2D(7, 7);
Index: source/graphics/tests/test_MapGenerator.h
===================================================================
--- source/graphics/tests/test_MapGenerator.h	(revision 23677)
+++ source/graphics/tests/test_MapGenerator.h	(working copy)
@@ -54,7 +54,7 @@
 
 			CMapGeneratorWorker worker(&scriptInterface);
 			worker.InitScriptInterface(0);
-			scriptInterface.LoadGlobalScriptFile(path);
+			TS_ASSERT(scriptInterface.LoadGlobalScriptFile(path));
 		}
 	}
 };

So it doesn't work more on Windows.

error(foo) prints a message, but thats not a JS exception, one can raise one using throw new Error("foo").
The serialization test error is unrelated and should be reported in a specific ticket, excluded from here.

Stan added a comment.EditedTue, May 19, 12:11 PM

Without patch

PS C:\Dev\trunk> .\binaries\system\test.exe
Running cxxtest tests (340 tests).......................................................................................................................................................................................................................................................................................................
In TestSerializer::test_Debug_types:
C:\Dev\trunk\source\simulation2\tests\test_Serializer.h:145: Error: Expected (std::string(stream.str()) == std::string("# comment\n" "i8: -123\n" "u8: 255\n" "i16: -12345\n" "u16: 56789\n" "i32: -123\n" "u32: 4294967173\n" "float: 1e+30\n" "double: 1.0000000000000001e+300\n" "fixed: 1234.5\n" "t: true\n" "f: false\n" "string: \"example\"\n" "string 2: \"example\\\"\\\\\\\"\"\n" "string 3: \"example\\n\\ntest\"\n" "string 4: \"t\xC3\xAAst\"\n" "raw bytes: (6 bytes) 00 01 02 03 0f 10\n")), found ("# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n" != "# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1.0000000000000001e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n")
............................................
Failed 1 and Skipped 0 of 340 tests
Success rate: 99%
[20100] HRT: activating HPET failed: Unknown error (-100022, 0xFFFFFFFFFFFE794A)
[20100] HRT: using name=TSC freq=2208000000.000000
[20100] HRT: counter=TSC freq=2.208e+09 res=4.52899e-10 bits=64
[20100] ERROR: JavaScript error: maps/random/tests/test_RectPlacer.js line 6
[20100] Error: heh
[20100]   @maps/random/tests/test_RectPlacer.js:6:1
[20100] 
[20100] (dumping stack frames may result in access violations...)
[20100]   locals: 1 w a 0 0 1 1
[20100] (done dumping stack frames)

With patch

PS C:\Dev\trunk> .\binaries\system\test.exe
Running cxxtest tests (340 tests).....
In TestMapGenerator::test_mapgen_scripts:
C:\Dev\trunk\source\graphics\tests\test_MapGenerator.h:57: Error: Assertion failed: scriptInterface.LoadGlobalScriptFile(path)
.................................................................................................................................................................................................................................................................................................
In TestSerializer::test_Debug_types:
C:\Dev\trunk\source\simulation2\tests\test_Serializer.h:145: Error: Expected (std::string(stream.str()) == std::string("# comment\n" "i8: -123\n" "u8: 255\n" "i16: -12345\n" "u16: 56789\n" "i32: -123\n" "u32: 4294967173\n" "float: 1e+30\n" "double: 1.0000000000000001e+300\n" "fixed: 1234.5\n" "t: true\n" "f: false\n" "string: \"example\"\n" "string 2: \"example\\\"\\\\\\\"\"\n" "string 3: \"example\\n\\ntest\"\n" "string 4: \"t\xC3\xAAst\"\n" "raw bytes: (6 bytes) 00 01 02 03 0f 10\n")), found ("# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n" != "# comment\ni8: -123\nu8: 255\ni16: -12345\nu16: 56789\ni32: -123\nu32: 4294967173\nfloat: 1e+30\ndouble: 1.0000000000000001e+300\nfixed: 1234.5\nt: true\nf: false\nstring: \"example\"\nstring 2: \"example\\\"\\\\\\\"\"\nstring 3: \"example\\n\\ntest\"\nstring 4: \"têst\"\nraw bytes: (6 bytes) 00 01 02 03 0f 10\n")
............................................
Failed 2 and Skipped 0 of 340 tests
Success rate: 99%
[31036] HRT: activating HPET failed: Unknown error (-100022, 0xFFFFFFFFFFFE794A)
[31036] HRT: using name=TSC freq=2208000000.000000
[31036] HRT: counter=TSC freq=2.208e+09 res=4.52899e-10 bits=64
[31036] ERROR: JavaScript error: maps/random/tests/test_RectPlacer.js line 6
[31036] Error: heh
[31036]   @maps/random/tests/test_RectPlacer.js:6:1
[31036] 
[31036] (dumping stack frames may result in access violations...)
[31036]   locals: 1 w a 0 0 1 1
[31036] (done dumping stack frames)

The serialization test error is unrelated and should be reported in a specific ticket, excluded from here.

It's already reported in rP23562 D2101 but sure I can create a ticket;

elexis requested changes to this revision.Fri, May 22, 11:31 AM

According to Stans test, on windows terminal, simulation tests show the filepath of the error, but not the JS exception thrown - unless using a debugger;
and with this patch it will not show the JS stack nor the JS filename of a failed rmgen test;
therefore it needs to show the filepath in a failed error message as with simulation tests
(if we want to support rmgen test development on windows without debugger).

Something like this line for simulation scripts.

TSM_ASSERT(L"Running script "+pathname.string(), scriptInterface.LoadScript(pathname, content));

With dbgview (warnings not showing in cmd or powershell)

This revision now requires changes to proceed.Fri, May 22, 11:31 AM
Stan added a comment.Fri, May 22, 1:04 PM

Isn't it a bit ouf scope? Literally no warning goes through on windows. We don't get any output. The patch is consistent with *all the behavior on windows.

elexis added a comment.EditedFri, May 22, 1:10 PM

From what you posted, the uploaded patch shows the filename of the failed JS randommap testfile on linux but not on windows, whereas failed simulation tests shows the filename of the JS test on both platforms when not using a debugger.

Stan added a comment.Fri, May 22, 1:52 PM

Ah right.