Bug 67080

Summary: Add unit test for existing StringBuilder
Product: WebKit Reporter: Xianzhu Wang <wangxianzhu>
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, aroben, darin, mitz, sullivan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch (requires the patch to bug 67079)
none
patch (requires the patch to bug 67079)
none
updated patch
none
fixed style problem
none
patch (added test for toString, and changed windows build file)
none
patch (added test for toString, and changed windows build file)
darin: review+, webkit.review.bot: commit-queue-
patch that can apply on ToT
wangxianzhu: commit-queue-
patch tentatively fixes windows build break
wangxianzhu: commit-queue-
patch (add three string sources into TestWebKitAPI.vcproj)
none
patch (add icu dependency in TestWebKitAPICommon.vsprops)
none
patch (removed Windows support)
none
patch (resolved conflicts with ToT) none

Xianzhu Wang
Reported 2011-08-26 20:45:23 PDT
Before refactoring StringBuilder, we need a unit test to keep the refactoring safe.
Attachments
patch (requires the patch to bug 67079) (3.80 KB, patch)
2011-08-31 20:27 PDT, Xianzhu Wang
no flags
patch (requires the patch to bug 67079) (9.17 KB, patch)
2011-08-31 20:30 PDT, Xianzhu Wang
no flags
updated patch (9.54 KB, patch)
2011-09-06 23:58 PDT, Xianzhu Wang
no flags
fixed style problem (9.25 KB, patch)
2011-09-07 00:05 PDT, Xianzhu Wang
no flags
patch (added test for toString, and changed windows build file) (6.62 KB, patch)
2011-09-13 23:19 PDT, Xianzhu Wang
no flags
patch (added test for toString, and changed windows build file) (10.56 KB, patch)
2011-09-13 23:25 PDT, Xianzhu Wang
darin: review+
webkit.review.bot: commit-queue-
patch that can apply on ToT (13.45 KB, patch)
2011-09-17 08:06 PDT, Xianzhu Wang
wangxianzhu: commit-queue-
patch tentatively fixes windows build break (15.91 KB, patch)
2011-09-18 23:17 PDT, Xianzhu Wang
wangxianzhu: commit-queue-
patch (add three string sources into TestWebKitAPI.vcproj) (13.94 KB, patch)
2011-09-19 01:44 PDT, Xianzhu Wang
no flags
patch (add icu dependency in TestWebKitAPICommon.vsprops) (14.70 KB, patch)
2011-09-19 03:33 PDT, Xianzhu Wang
no flags
patch (removed Windows support) (12.95 KB, patch)
2011-09-19 18:31 PDT, Xianzhu Wang
no flags
patch (resolved conflicts with ToT) (12.98 KB, patch)
2011-09-21 20:43 PDT, Xianzhu Wang
no flags
Xianzhu Wang
Comment 1 2011-08-31 20:27:41 PDT
Created attachment 105890 [details] patch (requires the patch to bug 67079)
Xianzhu Wang
Comment 2 2011-08-31 20:30:06 PDT
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
Comment 3 2011-09-06 23:58:57 PDT
Created attachment 106548 [details] updated patch
WebKit Review Bot
Comment 4 2011-09-07 00:02:01 PDT
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
Comment 5 2011-09-07 00:05:02 PDT
Created attachment 106549 [details] fixed style problem
Xianzhu Wang
Comment 6 2011-09-13 19:57:53 PDT
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
Comment 7 2011-09-13 23:19:01 PDT
Created attachment 107291 [details] patch (added test for toString, and changed windows build file)
Xianzhu Wang
Comment 8 2011-09-13 23:20:32 PDT
Comment on attachment 107291 [details] patch (added test for toString, and changed windows build file) Sorry. Wrong version.
Xianzhu Wang
Comment 9 2011-09-13 23:25:38 PDT
Created attachment 107292 [details] patch (added test for toString, and changed windows build file)
Darin Adler
Comment 10 2011-09-16 12:47:48 PDT
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 11 2011-09-16 12:50:19 PDT
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
Comment 12 2011-09-17 08:06:43 PDT
Created attachment 107769 [details] patch that can apply on ToT
Xianzhu Wang
Comment 13 2011-09-18 19:31:53 PDT
Comment on attachment 107769 [details] patch that can apply on ToT Investigating win build errors.
Xianzhu Wang
Comment 14 2011-09-18 21:11:33 PDT
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
Comment 15 2011-09-18 23:17:21 PDT
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 16 2011-09-19 01:06:01 PDT
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
Comment 17 2011-09-19 01:44:48 PDT
Created attachment 107816 [details] patch (add three string sources into TestWebKitAPI.vcproj)
Xianzhu Wang
Comment 18 2011-09-19 03:33:53 PDT
Created attachment 107827 [details] patch (add icu dependency in TestWebKitAPICommon.vsprops)
Xianzhu Wang
Comment 19 2011-09-19 18:31:34 PDT
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 20 2011-09-19 18:36:17 PDT
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
Comment 21 2011-09-21 20:43:58 PDT
Created attachment 108271 [details] patch (resolved conflicts with ToT)
WebKit Review Bot
Comment 22 2011-09-22 18:41:39 PDT
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
Comment 23 2011-09-22 18:41:45 PDT
All reviewed patches have been landed. Closing bug.
Adam Roben (:aroben)
Comment 24 2011-09-23 03:33:00 PDT
Thanks for writing more tests before changing the code! Could you please add the new file to TestWebKitAPI.vcproj, too?
Xianzhu Wang
Comment 25 2011-09-23 03:53:33 PDT
(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)
Comment 26 2011-09-23 05:56:45 PDT
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)
Comment 27 2011-09-23 10:41:36 PDT
I think the problem is that operator<< for WTF::String needs to be declared inside namespace WTF rather than outside of it.
Note You need to log in before you can comment on or make changes to this bug.