Details
Details
- Reviewers
elexis
- 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(); }
- Run the tests
- Verify the program has returned with non-zero status
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- badosu/fail-when-maps-fail
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 11535 Build 21130: Vulcan Build Jenkins Build 21129: Vulcan Build (macOS) Jenkins Build 21128: Vulcan Build (Windows) Jenkins Build 21127: arc lint + arc unit
Event Timeline
Comment Actions
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
Comment Actions
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
Comment Actions
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
Comment Actions
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. -.-
Comment Actions
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.
Comment Actions
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.
Comment Actions
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)
It's already reported in rP23562 D2101 but sure I can create a ticket;
Comment Actions
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)
Comment Actions
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.
Comment Actions
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.