Bug 67080 – Add unit test for existing StringBuilder

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Xianzhu Wang]

Before refactoring StringBuilder, we need a unit test to keep the refactoring safe.

[Xianzhu Wang]

Created attachment 105890 [details]
patch (requires the patch to bug 67079)

[Xianzhu Wang]

Created attachment 105891 [details]
patch (requires the patch to bug 67079)

Sorry I uploaded a wrong version of patch. This one is correct.

[Xianzhu Wang]

Created attachment 106548 [details]
updated patch

[WebKit Review Bot]

Attachment 106548 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/Te..." exit_code: 1

Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:685:  Missing "developmentRegion = English".  [xcodeproj/settings] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xianzhu Wang]

Created attachment 106549 [details]
fixed style problem

[Xianzhu Wang]

As there will be several changes to StringBuilder, I think it'd better if we have this change landed first. Then we can incrementally add unit tests together with the changes to StringBuilder. (Sorry for just mistakenly posting this comments to bug 67756.)

[Xianzhu Wang]

Created attachment 107291 [details]
patch (added test for toString, and changed windows build file)

[Xianzhu Wang]

Comment on attachment 107291 [details]
patch (added test for toString, and changed windows build file)

Sorry. Wrong version.

[Xianzhu Wang]

Created attachment 107292 [details]
patch (added test for toString, and changed windows build file)

[Darin Adler]

Comment on attachment 107292 [details]
patch (added test for toString, and changed windows build file)

Looks good. We can add even more coverage later.

[WebKit Review Bot]

Comment on attachment 107292 [details]
patch (added test for toString, and changed windows build file)

Rejecting attachment 107292 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Hunk #3 succeeded at 380 (offset 6 lines).
Hunk #4 succeeded at 590 (offset 6 lines).
2 out of 4 hunks FAILED -- saving rejects to file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj.rej
patching file Tools/TestWebKitAPI/Tests/WTF/StringBuilderBasic.cpp
patching file Tools/TestWebKitAPI/win/TestWebKitAPI.vcproj
Hunk #1 succeeded at 608 (offset 4 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9711697

[Xianzhu Wang]

Created attachment 107769 [details]
patch that can apply on ToT

[Xianzhu Wang]

Comment on attachment 107769 [details]
patch that can apply on ToT

Investigating win build errors.

[Xianzhu Wang]

Need help:
win-ews reports errors about unresolved externals, like:

6>StringBuilder.obj : error LNK2019: unresolved external symbol "public: class WTF::CString __thiscall WTF::String::utf8(bool)const " (?utf8@String@WTF@@QBE?AVCString@2@_N@Z) referenced in function "class std::basic_ostream<char,struct std::char_traits<char> > & __cdecl operator<<(class std::basic_ostream<char,struct std::char_traits<char> > &,class WTF::String const &)" (??6@YAAAV?$basic_ostream@DU?$char_traits@D@std@@@std@@AAV01@ABVString@WTF@@@Z)

I guess I should add the missing symbols into JavaScriptCore.def, but I'm wondering if I'm correct why they aren't in the def file, and if there are any drawbacks of adding them just for unit tests?

[Xianzhu Wang]

Created attachment 107806 [details]
patch tentatively fixes windows build break

Added symbols in JavaScriptCore.def. Not sure if this the correct solution, but uploaded it in case it is correct we can reduce interaction cycles due to the time difference.

[Xianzhu Wang]

Comment on attachment 107806 [details]
patch tentatively fixes windows build break

Adding the symbols breaks WebCore build on Windows. Saw AtomicString.cpp, StringImpl.cpp and WTFString.cpp explicitly included in WebCore.vcproj, so it seems that the symbols of them are intentionally not exported from JavaScriptCore. Should I add the three files into TestWebKitAPI.vcproj?

[Xianzhu Wang]

Created attachment 107816 [details]
patch (add three string sources into TestWebKitAPI.vcproj)

[Xianzhu Wang]

Created attachment 107827 [details]
patch (add icu dependency in TestWebKitAPICommon.vsprops)

[Xianzhu Wang]

Created attachment 107952 [details]
patch (removed Windows support)

I'm unable resolve the Windows build error by myself, as I haven't a Windows build environment. I'd like to leave this test not supported on Windows. As the other two tests under Tests/WTF, MetaAllocator and RedBlackTree, are also not supporting Windows, so I think this should be accepted (though I think this should be fixed.)

[Xianzhu Wang]

Comment on attachment 107952 [details]
patch (removed Windows support)

Darin, I still requested review for this new patch, because it has some changes since the review+ version.

[Xianzhu Wang]

Created attachment 108271 [details]
patch (resolved conflicts with ToT)

[WebKit Review Bot]

Comment on attachment 108271 [details]
patch (resolved conflicts with ToT)

Clearing flags on attachment: 108271

Committed r95770: <http://trac.webkit.org/changeset/95770>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Thanks for writing more tests before changing the code!

Could you please add the new file to TestWebKitAPI.vcproj, too?

[Xianzhu Wang]

(In reply to comment #24)
> Thanks for writing more tests before changing the code!
> 
> Could you please add the new file to TestWebKitAPI.vcproj, too?

Sorry I tried that but failed. You can find the details of the failures in the red "Win" EWS icons. I'm confused by the export stuffs and the way of the three string source files (WTFString.cpp, AtomicString.cpp and CString.cpp) are handled in WebCore.vcproj, and failed to resolve the linking issue of Tests/WTF/StringBuilder.cpp.

Filed bug 68688 about adding new unit tests into vcproj.

[Adam Roben (:aroben)]

This is breaking the build for me. I wonder why it isn't breaking it for the bots?


CompileC /Users/aroben/dev/BuildProducts/TestWebKitAPI.build/Debug/TestWebKitAPI.build/Objects-normal/x86_64/StringBuilder.o Tests/WTF/StringBuilder.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
    cd /Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI
    /Developer/usr/bin/clang -x c++ -arch x86_64 -fmessage-length=0 -fdiagnostics-print-source-range-info -fdiagnostics-show-category=id -fdiagnostics-parseable-fixits -Wno-trigraphs -fno-exceptions -fpascal-strings -O0 -Werror -Wno-return-type -Wparentheses -Wswitch -Wunused-function -Wno-unused-parameter -Wunused-variable -Wunused-value -DENABLE_DASHBOARD_SUPPORT -DWEBKIT_VERSION_MIN_REQUIRED=WEBKIT_VERSION_LATEST -fasm-blocks -Wno-deprecated-declarations -mmacosx-version-min=10.7 -gdwarf-2 -fvisibility=hidden -fvisibility-inlines-hidden -I/Users/aroben/dev/BuildProducts/TestWebKitAPI.build/Debug/TestWebKitAPI.build/TestWebKitAPI.hmap -I/Users/aroben/dev/BuildProducts/Debug/include -I/Users/aroben/dev/BuildProducts/Debug/WebCore.framework/PrivateHeaders/ForwardingHeaders -I/Users/aroben/dev/BuildProducts/Debug/WebCore.framework/PrivateHeaders/icu -I/Users/aroben/dev/BuildProducts/TestWebKitAPI.build/Debug/TestWebKitAPI.build/DerivedSources/x86_64 -I/Users/aroben/dev/BuildProducts/TestWebKitAPI.build/Debug/TestWebKitAPI.build/DerivedSources -Wall -W -Wno-unused-parameter -F/Users/aroben/dev/BuildProducts/Debug -F/System/Library/Frameworks/Quartz.framework/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -F/System/Library/Frameworks/CoreServices.framework/Frameworks -MMD -MT dependencies -MF /Users/aroben/dev/BuildProducts/TestWebKitAPI.build/Debug/TestWebKitAPI.build/Objects-normal/x86_64/StringBuilder.d -c /Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp -o /Users/aroben/dev/BuildProducts/TestWebKitAPI.build/Debug/TestWebKitAPI.build/Objects-normal/x86_64/StringBuilder.o
In file included from /Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:57:
In file included from /Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI/config.h:53:
In file included from /Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:31:
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/internal/gtest-internal.h:97:10:{97:10-97:13}: error: conversion from 'const WTF::String' to 'bool' is ambiguous [3]
   *os << val;
          ^~~
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest-message.h:122:5: note: in instantiation of function template specialization 'GTestStreamToHelper<WTF::String>' requested here [3]
     ::GTestStreamToHelper(ss_, val);
     ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:170:21: note: in instantiation of function template specialization 'testing::Message::operator<<<WTF::String>' requested here [3]
   return (Message() << streamable).GetString();
                     ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/internal/gtest-internal.h:230:10: note: in instantiation of function template specialization 'testing::internal::StreamableToString<WTF::String>' requested here [3]
   return StreamableToString(value);
          ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:1248:10: note: in instantiation of function template specialization 'testing::internal::FormatForFailureMessage<WTF::String>' requested here [3]
   return FormatForFailureMessage(value);
          ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:1273:20: note: in instantiation of function template specialization 'testing::internal::FormatForComparisonFailureMessage<WTF::String, WTF::String>' requested here [3]
                    FormatForComparisonFailureMessage(expected, actual),
                    ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:1299:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<WTF::String, WTF::String>' requested here [3]
     return CmpHelperEQ(expected_expression, actual_expression, expected,
            ^
/Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:49:5: note: in instantiation of function template specialization 'testing::internal::EqHelper<false>::Compare<WTF::String, WTF::String>' requested here [3]
     EXPECT_EQ(String(expected), String(builder.characters(), builder.length()));
     ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:1760:3: note: instantiated from:
   EXPECT_PRED_FORMAT2(::testing::internal:: \
   ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest_pred_impl.h:166:3: note: instantiated from:
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
   ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest_pred_impl.h:151:3: note: instantiated from:
   GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2),\
   ^
/Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:49:5: note: instantiated from:
     EXPECT_EQ(String(expected), String(builder.characters(), builder.length()));
     ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:1760:3: note: instantiated from:
   EXPECT_PRED_FORMAT2(::testing::internal:: \
   ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest_pred_impl.h:166:3: note: instantiated from:
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
   ^
/Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:49:5: note: instantiated from:
     EXPECT_EQ(String(expected), String(builder.characters(), builder.length()));
     ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:1760:3: note: instantiated from:
   EXPECT_PRED_FORMAT2(::testing::internal:: \
   ^
/Users/aroben/dev/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:49:5: note: instantiated from:
     EXPECT_EQ(String(expected), String(builder.characters(), builder.length()));
     ^
/Users/aroben/dev/BuildProducts/Debug/gtest.framework/Headers/gtest.h:1760:23: note: instantiated from:
   EXPECT_PRED_FORMAT2(::testing::internal:: \
                       ^
/Users/aroben/dev/BuildProducts/Debug/JavaScriptCore.framework/PrivateHeaders/WTFString.h:283:5: note: candidate function [3]
     operator UnspecifiedBoolTypeA() const;
     ^
/Users/aroben/dev/BuildProducts/Debug/JavaScriptCore.framework/PrivateHeaders/WTFString.h:284:5: note: candidate function [3]
     operator UnspecifiedBoolTypeB() const;
     ^
/usr/include/c++/4.2.1/ostream:176:23: note: passing argument to parameter '__n' here [3]
       operator<<(bool __n)
                       ^
1 error generated.

[Mark Rowe (bdash)]

I think the problem is that operator<< for WTF::String needs to be declared inside namespace WTF rather than outside of it.