Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stringify Variant compatible types for doctest output #40945

Merged
merged 1 commit into from
Aug 11, 2020
Merged

Stringify Variant compatible types for doctest output #40945

merged 1 commit into from
Aug 11, 2020

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Aug 1, 2020

See #40890 and #40940 for context.

Assertion messages should now be able to properly convert Variant to doctest strings (using VariantWriter aka var2str in GDScript), example output for Color:

D:\src\godot\tests\test_color.h(73): SUCCESS: CHECK( Color() != blue_html ) is correct!
  values: CHECK( Color( 0, 0, 0, 1 ) != Color( 0.25098, 0.376471, 1, 0.501961 ) )
  logged: Should not equal

Using VariantWriter is necessary if you want to copy and paste the output from doctest directly in code or even GDScript from them to be parsed back properly.

@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 1, 2020

Added a test case to see the output of all Variant types, but this results in a crash in all tests somehow (despite successful runs), lets see what I can find with sanitizer to investigate what causes it.

@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 1, 2020

Code that caused the crashes (commented out now as TODO):

Object *obj = memnew(Object);
INFO(obj);

Callable callable(obj, "has_method");
INFO(callable);

Signal signal(obj, "script_changed");
INFO(signal);
		
memdelete(obj);

Sanitizer output:

==4172==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000000270 at pc 0x00000f36e34a bp 0x7fffe7feb890 sp 0x7fffe7feb880
WRITE of size 4 at 0x606000000270 thread T0
    #0 0xf36e349 in atomic_decrement<unsigned int> core/safe_refcount.h:114
    #1 0xf36e349 in SafeRefCount::unref() core/safe_refcount.h:185
    #2 0xf36e349 in StringName::unref() core/string_name.cpp:87
    #3 0xf375164 in StringName::~StringName() core/string_name.cpp:378
    #4 0xf032fdb in ClassDB::ClassInfo::~ClassInfo() core/class_db.h:141
    #5 0xf04b9e4 in HashMap<StringName, ClassDB::ClassInfo, HashMapHasherDefault, HashMapComparatorDefault<StringName>, (unsigned char)3, (unsigned char)8>::Pair::~Pair() core/hash_map.h:61
    #6 0xf04ba7e in HashMap<StringName, ClassDB::ClassInfo, HashMapHasherDefault, HashMapComparatorDefault<StringName>, (unsigned char)3, (unsigned char)8>::Element::~Element() core/hash_map.h:72
    #7 0xf04bafa in void memdelete<HashMap<StringName, ClassDB::ClassInfo, HashMapHasherDefault, HashMapComparatorDefault<StringName>, (unsigned char)3, (unsigned char)8>::Element>(HashMap<StringName, ClassDB::ClassInfo, HashMapHasherDefault, HashMapComparatorDefault<StringName>, (unsigned char)3, (unsigned char)8>::Element*) core/os/memory.h:115
    #8 0xf03a33b in HashMap<StringName, ClassDB::ClassInfo, HashMapHasherDefault, HashMapComparatorDefault<StringName>, (unsigned char)3, (unsigned char)8>::clear() core/hash_map.h:511
    #9 0xf04f406 in HashMap<StringName, ClassDB::ClassInfo, HashMapHasherDefault, HashMapComparatorDefault<StringName>, (unsigned char)3, (unsigned char)8>::~HashMap() core/hash_map.h:547
    #10 0x7fb4f52f3040  (/lib/x86_64-linux-gnu/libc.so.6+0x43040)
    #11 0x7fb4f52f3139 in exit (/lib/x86_64-linux-gnu/libc.so.6+0x43139)
    #12 0x7fb4f52d1b9d in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b9d)
    #13 0x1976e09 in _start (/mnt/d/src/godot/bin/godot.linuxbsd.tools.64.4s+0x1976e09)

0x606000000270 is located 16 bytes inside of 64-byte region [0x606000000260,0x6060000002a0)
freed by thread T0 here:
    #0 0x7fb4f831e7b8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7b8)
    #1 0xf67ab72 in Memory::free_static(void*, bool) core/os/memory.cpp:169
    #2 0xf375673 in void memdelete<StringName::_Data>(StringName::_Data*) core/os/memory.h:118
    #3 0xf36db45 in StringName::cleanup() core/string_name.cpp:76
    #4 0x1a55589 in Main::test_entrypoint(int, char**, bool&) main/main.cpp:381
    #5 0x1976ff0 in main platform/linuxbsd/godot_linuxbsd.cpp:45
    #6 0x7fb4f52d1b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

previously allocated by thread T0 here:
    #0 0x7fb4f831eb50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
    #1 0xf679cb2 in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:76
    #2 0xf679bae in operator new(unsigned long, char const*) core/os/memory.cpp:41
    #3 0xf372c41 in StringName::StringName(String const&) core/string_name.cpp:276
    #4 0x69233fa in Object::_get_class_namev() const core/object.h:541
    #5 0xf218b98 in Object::_postinitialize() core/object.cpp:362
    #6 0xf247085 in postinitialize_handler(Object*) core/object.cpp:1838
    #7 0x1bac712 in Object* _post_initialize<Object>(Object*) core/os/memory.h:89
    #8 0x1bac712 in _DOCTEST_ANON_FUNC_270 tests/test_validate_testing.h:111
    #9 0x1b37b8b in doctest::Context::run() thirdparty/doctest/doctest.h:6118
    #10 0x1bb87e5 in test_main(int, char**) tests/test_main.cpp:92
    #11 0x1a55581 in Main::test_entrypoint(int, char**, bool&) main/main.cpp:380
    #12 0x1976ff0 in main platform/linuxbsd/godot_linuxbsd.cpp:45
    #13 0x7fb4f52d1b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

SUMMARY: AddressSanitizer: heap-use-after-free core/safe_refcount.h:114 in atomic_decrement<unsigned int>
Shadow bytes around the buggy address:
  0x0c0c7fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c0c7fff8000: fa fa fa fa 00 00 00 00 00 00 00 fa fa fa fa fa
  0x0c0c7fff8010: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
  0x0c0c7fff8020: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c7fff8030: fa fa fa fa fd fd fd fd fd fd fd fd fa fa fa fa
=>0x0c0c7fff8040: fd fd fd fd fd fd fd fd fa fa fa fa fd fd[fd]fd
  0x0c0c7fff8050: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c7fff8060: fa fa fa fa fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c0c7fff8070: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
  0x0c0c7fff8080: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c7fff8090: fa fa fa fa fd fd fd fd fd fd fd fd fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==4172==ABORTING

But this is out of scope of this PR, likely requires proper initialization of ClassDB, StringName, and singletons. I did try to initialize ClassDB but got similar crash. (CC @RevoluPowered might have a clue for this).

In either case this should be safe to merge now, no crashes from sanitizer with this PR in its current state either.

@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 1, 2020

Similar call stack in #40951...

@bruvzg
Copy link
Member

bruvzg commented Aug 1, 2020

Similar call stack in #40951...

Calling ClassDB::cleanup(); before exit is probably enough to fix this crash.

@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 1, 2020

Similar call stack in #40951...

Calling ClassDB::cleanup(); before exit is probably enough to fix this crash.

Yes this fixes it, I was lazy to find a matching deinit() method and/or assumed it's cleaned up automatically somewhere.

So, I do ClassDB::init(); and then ClassDB::cleanup(); only within the test case, which I think is the idea of unit testing. 🙂

I do think that for more complex stuff we could come up with more sophisticated mechanism to init and deinit stuff (especially all which requires a MainLoop).

@aaronfranke
Copy link
Member

Is this related to #34668?

@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 2, 2020

Is this related to #34668?

Current PR just teaches doctest how to convert those types for internal usage (printing to console, even generating output in XML with those Variant types properly serialized:

.\bin\godot --test --source-file="*test_validate*" --success --reporters=xml --out=doctest.txt
<?xml version="1.0" encoding="UTF-8"?>
<doctest binary="D:\src\godot\bin\godot.windows.tools.64.4" version="2.4.0">
  <Options order_by="name" rand_seed="0" first="0" last="4294967295" abort_after="5" subcase_filter_levels="2147483647" case_sensitive="false" no_throw="false" no_skip="false"/>
  <TestSuite name="Validate tests">
    <TestCase name="Always pass" filename="D:\src\godot\tests\test_validate_testing.h" line="39">
      <Expression success="true" type="CHECK" filename="D:\src\godot\tests\test_validate_testing.h" line="40">
        <Original>
          true
        </Original>
        <Expanded>
          true
        </Expanded>
      </Expression>
      <OverallResultsAsserts successes="1" failures="0"/>
    </TestCase>
    <TestCase name="Muting Godot error messages" filename="D:\src\godot\tests\test_validate_testing.h" line="49">
      <Expression success="true" type="CHECK" filename="D:\src\godot\tests\test_validate_testing.h" line="51">
        <Original>
          !_print_error_enabled
        </Original>
        <Expanded>
          true
        </Expanded>
        <Info>
          Error printing should be disabled.
        </Info>
      </Expression>
      <Expression success="true" type="CHECK" filename="D:\src\godot\tests\test_validate_testing.h" line="54">
        <Original>
          _print_error_enabled
        </Original>
        <Expanded>
          true
        </Expanded>
        <Info>
          Error printing should be re-enabled.
        </Info>
      </Expression>
      <OverallResultsAsserts successes="2" failures="0"/>
    </TestCase>
    <TestCase name="Pending tests are skipped" filename="D:\src\godot\tests\test_validate_testing.h" line="42" skipped="true"/>
    <TestCase name="Stringify Variant types" filename="D:\src\godot\tests\test_validate_testing.h" line="56">
      <Expression success="true" type="CHECK" filename="D:\src\godot\tests\test_validate_testing.h" line="187">
        <Original>
          true
        </Original>
        <Expanded>
          true
        </Expanded>
        <Info>
          null
        </Info>
        <Info>
          "Godot is finally here!"
        </Info>
        <Info>
          Vector2( 0.5, 1 )
        </Info>
        <Info>
          Vector2i( 1, 2 )
        </Info>
        <Info>
          Rect2( 0.5, 0.5, 100.5, 100.5 )
        </Info>
        <Info>
          Rect2i( 0, 0, 100, 100 )
        </Info>
        <Info>
          Vector3( 0.5, 1, 2 )
        </Info>
        <Info>
          Vector3i( 1, 2, 3 )
        </Info>
        <Info>
          Transform2D( 0.877583, 0.479426, -0.479426, 0.877583, 100, 100 )
        </Info>
        <Info>
          Plane( 1, 1, 1, 1 )
        </Info>
        <Info>
          Quat( 0.50819, 0.068284, 0.651417, 0.559228 )
        </Info>
        <Info>
          AABB( 0, 0, 0, 100, 100, 100 )
        </Info>
        <Info>
          Basis( 0.141986, -0.659178, 0.73846, 0.797984, -0.365203, -0.479426, 0.585715, 0.657351, 0.47416 )
        </Info>
        <Info>
          Transform( 0.141986, -0.659178, 0.73846, 0.797984, -0.365203, -0.479426, 0.585715, 0.657351, 0.47416, 0, 0, 0 )
        </Info>
        <Info>
          Color( 1, 0.5, 0.2, 0.3 )
        </Info>
        <Info>
          0000000000000001
        </Info>
        <Info>
          NodePath("godot/sprite")
        </Info>
        <Info/>
        <Info>
          000002A4B1934850
        </Info>
        <Info/>
        <Info/>
        <Info>
          {
"color": Color( 1, 0.5, 0.2, 0.3 ),
"string": "Godot is finally here!"
}
        </Info>
        <Info>
          [ "Godot is finally here!", Color( 1, 0.5, 0.2, 0.3 ) ]
        </Info>
        <Info>
          PackedByteArray( 0, 1, 2 )
        </Info>
        <Info>
          PackedInt32Array( 0, 1, 2 )
        </Info>
        <Info>
          PackedInt64Array( 0, 1, 2 )
        </Info>
        <Info>
          PackedFloat32Array( 0.5, 1.5, 2.5 )
        </Info>
        <Info>
          PackedFloat64Array( 0.5, 1.5, 2.5 )
        </Info>
        <Info>
          PackedStringArray( "Godot", "is", "finally", "here!" )
        </Info>
        <Info>
          PackedVector2Array( 0, 0, 1, 1, 2, 2 )
        </Info>
        <Info>
          PackedVector3Array( 0, 0, 0, 1, 1, 1, 2, 2, 2 )
        </Info>
        <Info>
          PackedColorArray( 0, 0, 0, 1, 1, 1, 1, 1, 2, 2, 2, 1 )
        </Info>
        <Info>
          doctest insertion operator &lt;&lt; null Vector2( 0.5, 1 ) Rect2( 0.5, 0.5, 100.5, 100.5 ) Color( 1, 0.5, 0.2, 0.3 )
        </Info>
      </Expression>
      <OverallResultsAsserts successes="1" failures="0"/>
    </TestCase>
  </TestSuite>
  <TestSuite>
    <TestCase name="[AStar] ABC path" filename="D:\src\godot\tests\test_astar.h" line="74" skipped="true"/>
    <TestCase name="[AStar] ABCX path" filename="D:\src\godot\tests\test_astar.h" line="83" skipped="true"/>
    <TestCase name="[AStar] Add/Remove" filename="D:\src\godot\tests\test_astar.h" line="93" skipped="true"/>
    <TestCase name="[Color] Constructor methods" filename="D:\src\godot\tests\test_color.h" line="40" skipped="true"/>
    <TestCase name="[Color] Conversion methods" filename="D:\src\godot\tests\test_color.h" line="116" skipped="true"/>
    <TestCase name="[Color] Manipulation methods" filename="D:\src\godot\tests\test_color.h" line="184" skipped="true"/>
    <TestCase name="[Color] Named colors" filename="D:\src\godot\tests\test_color.h" line="149" skipped="true"/>
    <TestCase name="[Color] Operators" filename="D:\src\godot\tests\test_color.h" line="74" skipped="true"/>
    <TestCase name="[Color] Reading methods" filename="D:\src\godot\tests\test_color.h" line="102" skipped="true"/>
    <TestCase name="[Color] Validation methods" filename="D:\src\godot\tests\test_color.h" line="169" skipped="true"/>
    <TestCase name="[Stress][AStar] Find paths" filename="D:\src\godot\tests\test_astar.h" line="221" skipped="true"/>
    <TestCase name="[String] ASCII" filename="D:\src\godot\tests\test_string.h" line="157" skipped="true"/>
    <TestCase name="[String] Assign from c-string (copycon)" filename="D:\src\godot\tests\test_string.h" line="62" skipped="true"/>
    <TestCase name="[String] Assign from c-widechar (copycon)" filename="D:\src\godot\tests\test_string.h" line="73" skipped="true"/>
    <TestCase name="[String] Assign from c-widechar (operator=)" filename="D:\src\godot\tests\test_string.h" line="68" skipped="true"/>
    <TestCase name="[String] Assign from cstr" filename="D:\src\godot\tests\test_string.h" line="51" skipped="true"/>
    <TestCase name="[String] Assign from string (operator=)" filename="D:\src\godot\tests\test_string.h" line="56" skipped="true"/>
    <TestCase name="[String] Begins with" filename="D:\src\godot\tests\test_string.h" line="246" skipped="true"/>
    <TestCase name="[String] Capitalize against many strings" filename="D:\src\godot\tests\test_string.h" line="590" skipped="true"/>
    <TestCase name="[String] Case function test" filename="D:\src\godot\tests\test_string.h" line="141" skipped="true"/>
    <TestCase name="[String] Checking string is empty when it should be" filename="D:\src\godot\tests\test_string.h" line="648" skipped="true"/>
    <TestCase name="[String] Comparisons (equal)" filename="D:\src\godot\tests\test_string.h" line="78" skipped="true"/>
    <TestCase name="[String] Comparisons (not equal)" filename="D:\src\godot\tests\test_string.h" line="85" skipped="true"/>
    <TestCase name="[String] Comparisons (operator &lt;)" filename="D:\src\godot\tests\test_string.h" line="92" skipped="true"/>
    <TestCase name="[String] Concatenation" filename="D:\src\godot\tests\test_string.h" line="99" skipped="true"/>
    <TestCase name="[String] Count and countn functionality" filename="D:\src\godot\tests\test_string.h" line="763" skipped="true"/>
    <TestCase name="[String] Cyrillic to_lower()" filename="D:\src\godot\tests\test_string.h" line="752" skipped="true"/>
    <TestCase name="[String] Erasing" filename="D:\src\godot\tests\test_string.h" line="225" skipped="true"/>
    <TestCase name="[String] Find and replace" filename="D:\src\godot\tests\test_string.h" line="182" skipped="true"/>
    <TestCase name="[String] IPVX address to string" filename="D:\src\godot\tests\test_string.h" line="563" skipped="true"/>
    <TestCase name="[String] Insertion" filename="D:\src\godot\tests\test_string.h" line="188" skipped="true"/>
    <TestCase name="[String] Number to string" filename="D:\src\godot\tests\test_string.h" line="194" skipped="true"/>
    <TestCase name="[String] Operator []" filename="D:\src\godot\tests\test_string.h" line="133" skipped="true"/>
    <TestCase name="[String] Slicing" filename="D:\src\godot\tests\test_string.h" line="216" skipped="true"/>
    <TestCase name="[String] String to float" filename="D:\src\godot\tests\test_string.h" line="207" skipped="true"/>
    <TestCase name="[String] String to integer" filename="D:\src\godot\tests\test_string.h" line="198" skipped="true"/>
    <TestCase name="[String] Substr" filename="D:\src\godot\tests\test_string.h" line="163" skipped="true"/>
    <TestCase name="[String] Testing for empty string" filename="D:\src\godot\tests\test_string.h" line="125" skipped="true"/>
    <TestCase name="[String] Testing size and length of string" filename="D:\src\godot\tests\test_string.h" line="113" skipped="true"/>
    <TestCase name="[String] UTF8" filename="D:\src\godot\tests\test_string.h" line="148" skipped="true"/>
    <TestCase name="[String] ensuring empty string into parse_utf8 passes empty string" filename="D:\src\godot\tests\test_string.h" line="747" skipped="true"/>
    <TestCase name="[String] find no case" filename="D:\src\godot\tests\test_string.h" line="176" skipped="true"/>
    <TestCase name="[String] lstrip and rstrip" filename="D:\src\godot\tests\test_string.h" line="677" skipped="true"/>
    <TestCase name="[String] sprintf" filename="D:\src\godot\tests\test_string.h" line="269" skipped="true"/>
    <TestCase name="[Variant] Writer and parser float" filename="D:\src\godot\tests\test_variant.h" line="79" skipped="true"/>
    <TestCase name="[Variant] Writer and parser integer" filename="D:\src\godot\tests\test_variant.h" line="41" skipped="true"/>
    <TestCase name="[string] Find" filename="D:\src\godot\tests\test_string.h" line="168" skipped="true"/>
  </TestSuite>
  <OverallResultsAsserts successes="4" failures="0"/>
  <OverallResultsTestCases successes="3" failures="0" skipped="48"/>
</doctest>

I do think it's better to use VariantWriter over simple stringification as in #34668, but yeah if I were to use String() operator conversions, #34668 would help the readability, but it's not used in this PR.

@akien-mga akien-mga added this to the 4.0 milestone Aug 11, 2020
@akien-mga akien-mga merged commit 23d07db into godotengine:master Aug 11, 2020
@akien-mga
Copy link
Member

Thanks!

@Xrayez Xrayez deleted the doctest-str-variant branch August 11, 2020 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants