| Summary: | Add unit test for existing StringBuilder | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||||||||||||||||||||||||
| Component: | Web Template Framework | Assignee: | 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
Xianzhu Wang
2011-08-26 20:45:23 PDT
Created attachment 105890 [details] patch (requires the patch to bug 67079) Created attachment 105891 [details] patch (requires the patch to bug 67079) Sorry I uploaded a wrong version of patch. This one is correct. Created attachment 106548 [details]
updated patch
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.
Created attachment 106549 [details]
fixed style problem
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.) Created attachment 107291 [details]
patch (added test for toString, and changed windows build file)
Comment on attachment 107291 [details]
patch (added test for toString, and changed windows build file)
Sorry. Wrong version.
Created attachment 107292 [details]
patch (added test for toString, and changed windows build file)
Comment on attachment 107292 [details]
patch (added test for toString, and changed windows build file)
Looks good. We can add even more coverage later.
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 Created attachment 107769 [details]
patch that can apply on ToT
Comment on attachment 107769 [details]
patch that can apply on ToT
Investigating win build errors.
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? 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.
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?
Created attachment 107816 [details]
patch (add three string sources into TestWebKitAPI.vcproj)
Created attachment 107827 [details]
patch (add icu dependency in TestWebKitAPICommon.vsprops)
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.)
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.
Created attachment 108271 [details]
patch (resolved conflicts with ToT)
Comment on attachment 108271 [details] patch (resolved conflicts with ToT) Clearing flags on attachment: 108271 Committed r95770: <http://trac.webkit.org/changeset/95770> All reviewed patches have been landed. Closing bug. Thanks for writing more tests before changing the code! Could you please add the new file to TestWebKitAPI.vcproj, too? (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. 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.
I think the problem is that operator<< for WTF::String needs to be declared inside namespace WTF rather than outside of it. |