The version of googletest we use for TestWebKitAPI is version 1.5.0, which is pretty old at this point. We should update to the latest (1.7.0 as of this filing) and has some features that would be useful, including the ability to use EXPECT_EQ on types that do not have an operator<< specified.
Created attachment 320105 [details] Patch
Thanks Yusuke!
Created attachment 320249 [details] Patch
Created attachment 320284 [details] Patch
Comment on attachment 320284 [details] Patch When importing a whole directory, I don’t think the ChangeLog should list the individual files as if we were doing the programming as part of this project. Just one big one like this should do: gtest: Replaced with a new version. If changes to our code for compatibility with the new gtest were needed, for those we should use the normal change log style.
Comment on attachment 320284 [details] Patch Marking r- since the patch breaks the build.
I wonder if this will mean we can remove the GTEST_HAS_TR1_TUPLE=0 hack that is in a few places in our project.
It sounds like there might be a 1.8.1 release in the near future so perhaps we should revisit this work. https://github.com/google/googletest/issues/1079#issuecomment-412532869 After that the 1.8.1 branch goes into maintenance and a 1.9 release will drop pre C++11 support. https://github.com/google/googletest/issues/1366
Created attachment 348556 [details] Patch
Comment on attachment 348556 [details] Patch This updates gtest to HEAD and ensures that we don't stick any WebKit stuff into gtest itself this time. In doing so, there was just one missing include in WebCore that blocked compilation -- otherwise it was as simple as adding wtf/Assertions.h (and FastMalloc.h and DisallowCType.h for good measure) to TestWebKitAPI/config.h.
Created attachment 348561 [details] Patch
Confirmed locally that Mac and WinCairo API tests all pass locally with this patch. :D
This WPE EWS bot is currently borked. Its giving the same errors across different patches. Michael could you maybe make sure GTK and WPE run API tests successfully?
(In reply to Tim Horton from comment #7) > I wonder if this will mean we can remove the GTEST_HAS_TR1_TUPLE=0 hack that > is in a few places in our project. Regarding this comment, the only place I see this occurring is here: https://github.com/WebKit/webkit/blob/master/Tools/TestWebKitAPI/Configurations/Base.xcconfig#L38 But I believe we can remove GTEST_HAS_TR1_TUPLE=0 as well as GTEST_HAS_RTTI=0 there, as these were also removed from gtest/xcode/Config/General.xcconfig.
Created attachment 348573 [details] Patch
(In reply to Don Olmstead from comment #13) > This WPE EWS bot is currently borked. Its giving the same errors across > different patches. > > Michael could you maybe make sure GTK and WPE run API tests successfully? The buildbot (separate from EWS) is broken since r235531 "[CMake] Replace AVFoundationSupport.py using CMake" and is showing the same error. So I think the EWS bot is fine, and the build is just broken. I have no clue why it broke, maybe one of the changes in WebKitFeatures.cmake?
(In reply to Michael Catanzaro from comment #16) > (In reply to Don Olmstead from comment #13) > > This WPE EWS bot is currently borked. Its giving the same errors across > > different patches. > > > > Michael could you maybe make sure GTK and WPE run API tests successfully? > > The buildbot (separate from EWS) is broken since r235531 "[CMake] Replace > AVFoundationSupport.py using CMake" and is showing the same error. So I > think the EWS bot is fine, and the build is just broken. > > I have no clue why it broke, maybe one of the changes in > WebKitFeatures.cmake? Weird I swore previous patches for that one passed. Is the bundling of the unified build deterministic? There’s always a chance that if the sets change that there can be a build breakage.
It is deterministic, but yes if files are added or removed that will change the sets.
Created attachment 348597 [details] Patch
(In reply to Ross Kirsling from comment #19) > Created attachment 348597 [details] > Patch Resubmitted now that WPE is stable again.
I believe this is ready for a review. We can follow up with whatever changes happen between this point in gtest master and 1.8.1. There hasn't been a release to gtest in over 2 years and they keep teasing a release soon but who realistically knows. I would assume there will be a 1.8.1 release right after we land this of course because that's how the world works.
Comment on attachment 348597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348597&action=review Overall this patch looks fine, but I think the Project file changes in ThirdParty/gtest are wrong. It seems to lose our Production build target, and does not understand that we might build with our Internal SDK versus the external developer's SDK. I can't approve the patch in this form. > Source/ThirdParty/gtest/xcode/Config/DebugProject.xcconfig:-39 > -SDKROOT_YES = macosx.internal; I don't think we want this change. > Source/ThirdParty/gtest/xcode/Config/FrameworkTarget.xcconfig:-19 > -INSTALL_PATH = $(SYSTEM_LIBRARY_DIR)/PrivateFrameworks; I don't think we want this change. > Source/ThirdParty/gtest/xcode/Config/General.xcconfig:-19 > -ARCHS = $(ARCHS_STANDARD_32_64_BIT); I don't think we want this change. > Source/ThirdParty/gtest/xcode/Config/General.xcconfig:20 > +WARNING_CFLAGS = -Wall -Werror -Wendif-labels -Wnewline-eof -Wno-sign-compare -Wshadow I really don't think we want these changes for the macOS/iOS port. > Source/ThirdParty/gtest/xcode/Config/ProductionProject.xcconfig:-34 > -WK_QUOTED_OVERRIDE_FRAMEWORKS_DIR_YES = "$(WK_OVERRIDE_FRAMEWORKS_DIR)"; Why is this file going away? > Source/ThirdParty/gtest/xcode/Config/ReleaseProject.xcconfig:-41 > -SDKROOT_YES = macosx.internal; I don't think we want this change. > Source/ThirdParty/gtest/xcode/Config/StaticLibraryTarget.xcconfig:-20 > -OTHER_LDFLAGS = ; I don't think we want this change. > Source/ThirdParty/gtest/xcode/Makefile:-2 > -include ../../../../Makefile.shared I don't think we want this change. > Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-294 > - }; Why is this going away? > Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-319 > - 1A0A4C4614B7A3BC00895135 /* Frameworks */, Why is this going away? > Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-347 > - }; Ditto. > Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:841 > + GCC_VERSION = com.apple.compilers.llvm.clang.1_0; GCC_VERSION should be controlled by our xcconfig files. I don't want it set individually! > Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:967 > + SDKROOT = macosx; We don't want this set in the project file. It should be controlled by the xcconfig file. > Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:-1026 > - }; Why are you taking away our Production build target? > Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:1096 > + defaultConfigurationName = Release; Are all of these changes from Production to Release expected? I am a little surprised to see this.
Created attachment 348700 [details] Patch
Created attachment 348701 [details] Patch
(In reply to Brent Fulgham from comment #22) > Comment on attachment 348597 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348597&action=review > > Overall this patch looks fine, but I think the Project file changes in > ThirdParty/gtest are wrong. It seems to lose our Production build target, > and does not understand that we might build with our Internal SDK versus the > external developer's SDK. I can't approve the patch in this form. Sorry, I didn't know the files in that directory were custom. I've now gone through them all manually and believe existing functionality should be preserved.
There's been a legit release of gtest. https://github.com/google/googletest/releases/tag/release-1.8.1
Created attachment 348740 [details] Patch
Created attachment 348741 [details] Patch
Created attachment 348745 [details] Patch
Man, it sure doesn't like it when I include the runtests.sh mode updates... Oh well, we don't need them anyway.
Note that the WinCairo EWS failure here is a sporadically-occurring fluke (bug 187725). Stepping forward from the earlier hash to the 1.8.1 release was a relatively trivial update to the patch here.
Comment on attachment 348745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348745&action=review This patch looks like it has correct the project file issues I was concerned about. I think this looks fine. > Source/ThirdParty/gtest/README.WebKit:-16 > - gtest-md.vcproj upgraded to VS 2005 (8.0) format to match the rest of WebKit Were these changes rolled into gtest proper?
Comment on attachment 348745 [details] Patch rs=me under the assumption that the Sony folks will fix the WinCairo build failure.
Thanks Brent!
Comment on attachment 348745 [details] Patch Clearing flags on attachment: 348745 Committed r235613: <https://trac.webkit.org/changeset/235613>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44100989>