Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[ios] Fix default assets url #46214

Merged
auto-submit merged 2 commits into from
Sep 23, 2023
Merged

[ios] Fix default assets url #46214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90473d9
use correct asset path
Sep 22, 2023
9459ef4
format
Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
}

NSString* FLTAssetPath(NSBundle* bundle) {
return [bundle objectForInfoDictionaryKey:@"FLTAssetsPath"] ?: @"flutter_assets";
return [bundle objectForInfoDictionaryKey:@"FLTAssetsPath"] ?: kDefaultAssetPath;
Copy link
Contributor Author

@cyanglaz cyanglaz Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually going to fail the g3 roll again. It turned out I was wrong about changing the API to using NSString fixing the issue. The root cause is when the full relative path is used, unloaded-bundle returns a nil path for assets. we have to use "flutter_assets" directly for unloaded-bundles.

Because I reverted to @"flutter_assets" in #46073, the roll was fixed.

The fix should be trying to load from kDefaultAssetPath first, if not successful, then try @"flutter_assets" for unloaded bundles.

I will create a PR to fix this with an integration test.

cc @stuartmorgan @XilaiZhang

Copy link
Contributor

@XilaiZhang XilaiZhang Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this was already rolled in by @CaseyHillers

Copy link
Contributor Author

@cyanglaz cyanglaz Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Maybe it works for XCUITests but not for app extension? That's good to know, we need to fix it for app extensions anyway

}

NSString* FLTAssetsPathFromBundle(NSBundle* bundle) {
Expand Down
106 changes: 104 additions & 2 deletions shell/platform/darwin/ios/framework/Source/FlutterDartProjectTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,116 @@ - (void)testFLTAssetsURLFromBundle {
id mockBundle = OCMClassMock([NSBundle class]);
id mockMainBundle = OCMPartialMock([NSBundle mainBundle]);
NSString* resultAssetsPath = @"path/to/foo/assets";
OCMStub([mockBundle pathForResource:@"flutter_assets" ofType:@""]).andReturn(nil);
OCMStub([mockMainBundle pathForResource:@"flutter_assets" ofType:@""])
OCMStub([mockBundle pathForResource:@"Frameworks/App.framework/flutter_assets" ofType:@""])
.andReturn(nil);
OCMStub([mockMainBundle pathForResource:@"Frameworks/App.framework/flutter_assets" ofType:@""])
.andReturn(resultAssetsPath);
NSString* path = FLTAssetsPathFromBundle(mockBundle);
XCTAssertEqualObjects(path, @"path/to/foo/assets");
}
}

- (void)testFLTAssetPathReturnsTheCorrectValue {
{
// Found assets path in info.plist
id mockBundle = OCMClassMock([NSBundle class]);
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(@"foo/assets");
XCTAssertEqualObjects(FLTAssetPath(mockBundle), @"foo/assets");
}
{
// No assets path in info.plist, use default value
id mockBundle = OCMClassMock([NSBundle class]);
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(nil);
XCTAssertEqualObjects(FLTAssetPath(mockBundle), kDefaultAssetPath);
}
}

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are both of a private helper method; there's nothing about the structure here that would indicate that someone making a change like your recent change couldn't just change these tests if they broke.

The deeper problem is that we appear to have no tests of the public lookupKeyForAsset:* methods in this file. Could you add tests for each variant of that, with comments on the expectations saying that they are testing public API behaviors and anything that breaks the expectations is likely to break plugins?

- (void)testLookUpForAssets {
{
id mockBundle = OCMPartialMock([NSBundle mainBundle]);
// Found assets path in info.plist
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(@"foo/assets");
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar"];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath, @"foo/assets/bar");
[mockBundle stopMocking];
}
{
id mockBundle = OCMPartialMock([NSBundle mainBundle]);
// No assets path in info.plist, use default value
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(nil);
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar"];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath, @"Frameworks/App.framework/flutter_assets/bar");
[mockBundle stopMocking];
}
}

- (void)testLookUpForAssetsFromBundle {
{
id mockBundle = OCMClassMock([NSBundle class]);
// Found assets path in info.plist
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(@"foo/assets");
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar" fromBundle:mockBundle];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath, @"foo/assets/bar");
}
{
// No assets path in info.plist, use default value
id mockBundle = OCMClassMock([NSBundle class]);
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(nil);
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar" fromBundle:mockBundle];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath, @"Frameworks/App.framework/flutter_assets/bar");
}
}

- (void)testLookUpForAssetsFromPackage {
{
id mockBundle = OCMPartialMock([NSBundle mainBundle]);
// Found assets path in info.plist
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(@"foo/assets");
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar" fromPackage:@"bar_package"];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath, @"foo/assets/packages/bar_package/bar");
[mockBundle stopMocking];
}
{
id mockBundle = OCMPartialMock([NSBundle mainBundle]);
// No assets path in info.plist, use default value
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(nil);
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar" fromPackage:@"bar_package"];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath,
@"Frameworks/App.framework/flutter_assets/packages/bar_package/bar");
[mockBundle stopMocking];
}
}

- (void)testLookUpForAssetsFromPackageFromBundle {
{
id mockBundle = OCMClassMock([NSBundle class]);
// Found assets path in info.plist
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(@"foo/assets");
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar"
fromPackage:@"bar_package"
fromBundle:mockBundle];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath, @"foo/assets/packages/bar_package/bar");
}
{
id mockBundle = OCMClassMock([NSBundle class]);
// No assets path in info.plist, use default value
OCMStub([mockBundle objectForInfoDictionaryKey:@"FLTAssetsPath"]).andReturn(nil);
NSString* assetsPath = [FlutterDartProject lookupKeyForAsset:@"bar"
fromPackage:@"bar_package"
fromBundle:mockBundle];
// This is testing public API, changing this assert is likely to break plugins.
XCTAssertEqualObjects(assetsPath,
@"Frameworks/App.framework/flutter_assets/packages/bar_package/bar");
}
}

- (void)testDisableImpellerSettingIsCorrectlyParsed {
id mockMainBundle = OCMPartialMock([NSBundle mainBundle]);
OCMStub([mockMainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]).andReturn(@"NO");
Expand Down