WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
67080
Add unit test for existing StringBuilder
https://bugs.webkit.org/show_bug.cgi?id=67080
Summary
Add unit test for existing StringBuilder
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
Details
Formatted Diff
Diff
patch (requires the patch to bug 67079)
(9.17 KB, patch)
2011-08-31 20:30 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
updated patch
(9.54 KB, patch)
2011-09-06 23:58 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
fixed style problem
(9.25 KB, patch)
2011-09-07 00:05 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
patch (added test for toString, and changed windows build file)
(6.62 KB, patch)
2011-09-13 23:19 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
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-
Details
Formatted Diff
Diff
patch that can apply on ToT
(13.45 KB, patch)
2011-09-17 08:06 PDT
,
Xianzhu Wang
wangxianzhu
: commit-queue-
Details
Formatted Diff
Diff
patch tentatively fixes windows build break
(15.91 KB, patch)
2011-09-18 23:17 PDT
,
Xianzhu Wang
wangxianzhu
: commit-queue-
Details
Formatted Diff
Diff
patch (add three string sources into TestWebKitAPI.vcproj)
(13.94 KB, patch)
2011-09-19 01:44 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
patch (add icu dependency in TestWebKitAPICommon.vsprops)
(14.70 KB, patch)
2011-09-19 03:33 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
patch (removed Windows support)
(12.95 KB, patch)
2011-09-19 18:31 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
patch (resolved conflicts with ToT)
(12.98 KB, patch)
2011-09-21 20:43 PDT
,
Xianzhu Wang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug