Bug 208260 – TestWebKitAPI and WebKitTestRunner should have bundle identifiers

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

This is useful for testing layout and api cases where a bundle identifier is needed, so we don't have to manually set it each time.

[Radar WebKit Bug Importer]

<rdar://problem/59820107>

[Kate Cheney]

Created attachment 391779 [details]
Patch

[Tim Horton]

Comment on attachment 391779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391779&action=review

> Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig:27
> +PRODUCT_BUNDLE_IDENTIFIER = com.apple.TestWebKitAPI;

I wonder if these should be com.apple.WebKit.* like the subprocesses are

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5333
> +				CREATE_INFOPLIST_SECTION_IN_BINARY = YES;
> +				INFOPLIST_FILE = "$(SRCROOT)/Info.plist";

You should move these two lines to the xcconfig, not here.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5342
> +				CREATE_INFOPLIST_SECTION_IN_BINARY = YES;
> +				INFOPLIST_FILE = "$(SRCROOT)/Info.plist";

Ditto

> Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:1291
> +				CREATE_INFOPLIST_SECTION_IN_BINARY = YES;
> +				INFOPLIST_FILE = "$(SRCROOT)/Info.plist";

Ditto etc etc etc

[Kate Cheney]

Created attachment 391784 [details]
Patch for landing

[Kate Cheney]

(In reply to Tim Horton from comment #3)
> Comment on attachment 391779 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391779&action=review
> 
> > Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig:27
> > +PRODUCT_BUNDLE_IDENTIFIER = com.apple.TestWebKitAPI;
> 
> I wonder if these should be com.apple.WebKit.* like the subprocesses are
> 

Good call.

> > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5333
> > +				CREATE_INFOPLIST_SECTION_IN_BINARY = YES;
> > +				INFOPLIST_FILE = "$(SRCROOT)/Info.plist";
> 
> You should move these two lines to the xcconfig, not here.
> 
> > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5342
> > +				CREATE_INFOPLIST_SECTION_IN_BINARY = YES;
> > +				INFOPLIST_FILE = "$(SRCROOT)/Info.plist";
> 
> Ditto
> 
> > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:1291
> > +				CREATE_INFOPLIST_SECTION_IN_BINARY = YES;
> > +				INFOPLIST_FILE = "$(SRCROOT)/Info.plist";
> 
> Ditto etc etc etc


Done! Thanks Tim.

[WebKit Commit Bot]

Comment on attachment 391784 [details]
Patch for landing

Clearing flags on attachment: 391784

Committed r257522: <https://trac.webkit.org/changeset/257522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change caused 16 API test failures on the bots:
https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/3268

Please let API test EWS bots complete before re-landing.

[Ryan Haddad]

Reverted r257522 for reason:

Caused 16 API test failures on the bots

Committed r257540: <https://trac.webkit.org/changeset/257540>

[Kate Cheney]

Created attachment 391872 [details]
Patch

[Kate Cheney]

Letting EWS run. 

Adding a bundle ID changes the file path for TestWebKitAPI website data, so this patch updates the expectations for tests which need to know it. There should probably be a central place where tests can construct the default path the same way the default WebsiteDataStore does instead of hardcoding it each time. That would avoid test failures like these if the default path changes in the future.

[Jonathan Bedard]

(In reply to katherine_cheney from comment #11)
> Letting EWS run. 
> 
> Adding a bundle ID changes the file path for TestWebKitAPI website data, so
> this patch updates the expectations for tests which need to know it. There
> should probably be a central place where tests can construct the default
> path the same way the default WebsiteDataStore does instead of hardcoding it
> each time. That would avoid test failures like these if the default path
> changes in the future.

We should be careful about this. That 'central place' needs to be in testing code, not WebKit, because we have had bugs where we used the wrong default data store, and we want API tests to catch that sort of thing.

[Kate Cheney]

Created attachment 391911 [details]
Patch

[Kate Cheney]

Created attachment 391918 [details]
Patch

[Tim Horton]

Comment on attachment 391918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391918&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:456
> +    NSURL *defaultResourceLoadStatisticsFilePath = [NSURL fileURLWithPath:[@"~/Library/WebKit/com.apple.WebKit.TestWebKitAPI/WebsiteData/ResourceLoadStatistics/full_browsing_session_resourceLog.plist" stringByExpandingTildeInPath] isDirectory:NO];

I wonder if we should have a helper for "give me a path inside TWKAPI's library dir"

> Tools/TestWebKitAPI/Tests/mac/SetAndUpdateCacheModel.mm:49
> +static void resetTestState()
> +{
> +    WebPreferences *standardPreferences = [WebPreferences standardPreferences];
> +    [standardPreferences setCacheModel:WebCacheModelDocumentViewer];
> +}
> +
>  TEST(WebKitLegacy, SetAndUpdateCacheModelInitialModel)
>  {
> +    resetTestState();
> +
>      EXPECT_EQ((int)WebCacheModelDocumentViewer, (int)[WebView _cacheModel]);
>  

I think the point of this test is to test the default value, so it's wrong to set it to something beforehand.

Can you instead /delete/ the default, so we fall back to the default-default?

[Tim Horton]

(In reply to Tim Horton from comment #15)
> Comment on attachment 391918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391918&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:456
> > +    NSURL *defaultResourceLoadStatisticsFilePath = [NSURL fileURLWithPath:[@"~/Library/WebKit/com.apple.WebKit.TestWebKitAPI/WebsiteData/ResourceLoadStatistics/full_browsing_session_resourceLog.plist" stringByExpandingTildeInPath] isDirectory:NO];
> 
> I wonder if we should have a helper for "give me a path inside TWKAPI's
> library dir"

... oh, you already said that

[Kate Cheney]

(In reply to Tim Horton from comment #15)
> Comment on attachment 391918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391918&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:456
> > +    NSURL *defaultResourceLoadStatisticsFilePath = [NSURL fileURLWithPath:[@"~/Library/WebKit/com.apple.WebKit.TestWebKitAPI/WebsiteData/ResourceLoadStatistics/full_browsing_session_resourceLog.plist" stringByExpandingTildeInPath] isDirectory:NO];
> 
> I wonder if we should have a helper for "give me a path inside TWKAPI's
> library dir"
> 
> > Tools/TestWebKitAPI/Tests/mac/SetAndUpdateCacheModel.mm:49
> > +static void resetTestState()
> > +{
> > +    WebPreferences *standardPreferences = [WebPreferences standardPreferences];
> > +    [standardPreferences setCacheModel:WebCacheModelDocumentViewer];
> > +}
> > +
> >  TEST(WebKitLegacy, SetAndUpdateCacheModelInitialModel)
> >  {
> > +    resetTestState();
> > +
> >      EXPECT_EQ((int)WebCacheModelDocumentViewer, (int)[WebView _cacheModel]);
> >  
> 
> I think the point of this test is to test the default value, so it's wrong
> to set it to something beforehand.
> 
> Can you instead /delete/ the default, so we fall back to the default-default?

Yes. Very good idea.

[Kate Cheney]

Created attachment 391933 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391933 [details]
Patch

Clearing flags on attachment: 391933

Committed r257614: <https://trac.webkit.org/changeset/257614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.