Skip to content

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Mar 5, 2024

Description

This PR makes the test assets directory content available to web tests.
The goal of this PR is to unblock #144314 which activates ink sparkle animation on CanvasKit. The ink sparkle animation relies on a shader.

#144314 works when launching a flutter app but fails when launching a web test with a 404 error for the ink_sparkle.frag file.

Related Issue

Needed for #144314.

Tests

Adds 1 test.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 5, 2024
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

ink_sparkle should be loaded via asset bundles. if the asset bundle is broken, then a fix should be made there, not by adjusting the test harness loading behavior.

@bleroux
Copy link
Contributor Author

bleroux commented Mar 5, 2024

@jonahwilliams
Ink sparkle is correctly loaded when launching an app using 'flutter run -d chrome' (once activated on canvas kit, see the PR linked on the description). That's why I assumed, it was a test only issue.
Can you point me to someone who can work on this? Or give my guidance on where to fix this?

@jonahwilliams
Copy link
Contributor

The test assets are built here:

https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/commands/test.dart#L656-L680

maybe those arguments need to be adjusted for web tests?

@bleroux bleroux force-pushed the flutter_tools_test_web_makes_ink_sparkle_frag_available branch 3 times, most recently from 03a85a4 to e8a5b36 Compare March 7, 2024 18:59
@bleroux bleroux changed the title [flutter_tools] Makes ink_sparkle.frag available for web tests [flutter_tools] Make test asset directory visible on web platform testing Mar 7, 2024
@bleroux
Copy link
Contributor Author

bleroux commented Mar 7, 2024

The test assets are built here:

https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/commands/test.dart#L656-L680

maybe those arguments need to be adjusted for web tests?

Thanks for pointing out this code.
I figured out that test assets are correctly generated for web tests (this code is called before launching the internal web server). But the asset directory was not sent to FlutterWebPlatform.
I have updated the PR to make the test assets directory visible on the internal web server (and I added a test).
Let me know it this is the right way or if there was a reason for not exposing this directory?

@bleroux bleroux force-pushed the flutter_tools_test_web_makes_ink_sparkle_frag_available branch from e8a5b36 to 9e492b2 Compare March 7, 2024 19:31
@bleroux bleroux requested a review from jonahwilliams March 7, 2024 20:23
@bleroux bleroux force-pushed the flutter_tools_test_web_makes_ink_sparkle_frag_available branch 3 times, most recently from 5dfa177 to 6570f03 Compare March 19, 2024 20:04
@bleroux bleroux force-pushed the flutter_tools_test_web_makes_ink_sparkle_frag_available branch 4 times, most recently from 4d13643 to 08333b1 Compare March 27, 2024 18:06
@bleroux bleroux force-pushed the flutter_tools_test_web_makes_ink_sparkle_frag_available branch from 08333b1 to 3c3ce41 Compare March 28, 2024 16:17
@bleroux
Copy link
Contributor Author

bleroux commented Mar 28, 2024

@christopherfujino
Can you point me to someone who can review this PR?
Based on Jonah feedback I updated this PR some weeks ago.
Thanks!

@christopherfujino
Copy link
Contributor

I'm gonna defer to @andrewkolos to make the final review on this one, he's currently out of office but will be back ~1 week.

@andrewkolos
Copy link
Contributor

Hello, I'm back and trying to catch up on this. I'm starting out from the same place @jonahwilliams did:

ink_sparkle should be loaded via [the] asset bundle. if the asset bundle is broken, then a fix should be made there, not by adjusting the test harness loading behavior.

I need to figure out where this shader is being loaded, how it's being loaded, and why it's not working for web tests.

@bleroux
Copy link
Contributor Author

bleroux commented Apr 4, 2024

@andrewkolos
Hey! For context, before Jonah review, my first implementation was to add a specifc rule to the Shelf server in order to serve ink_sparkle file.
That's why Jonah commented:

ink_sparkle should be loaded via [the] asset bundle. if the asset bundle is broken, then a fix should be made there, not by adjusting the test harness loading behavior.

Since then I updated the PR to serve the test asset directory which was already correctly built for web tests but whose path was not known by the FlutterWebPlatform.

@bleroux
Copy link
Contributor Author

bleroux commented Apr 15, 2024

@andrewkolos Did you get a chance to look at this PR?

@Piinks Piinks added the team-tool Owned by Flutter Tool team label Apr 18, 2024
@andrewkolos
Copy link
Contributor

Hello, I got preoccupied with other things, but I have no excuse for not communicating this. My apologies.

Since then I updated the PR to serve the test asset directory which was already correctly built for web tests but whose path was not known by the FlutterWebPlatform.

If the path was not known by FlutterWebPlatform, wouldn't any test that depends on an asset fail when running on a browser? To my knowledge, this is not the case, and I've verified this locally1 (assuming I did so correctly). I'm missing something here.

Footnotes

  1. I created a simple Flutter app that displays text loaded from an asset file. I wrote an integration test that verifies that the text is displayed and I ran this test on chrome. The test passed.

@andrewkolos
Copy link
Contributor

To extend the previous comment: I believe the code that actually loads ink_sparkle.frag is unique compared to the more general asset-loading code, but I have to imagine that the file would still be visible on the web server hosted by flutter test since it's bundled in the same directory as any other asset file.

@bleroux
Copy link
Contributor Author

bleroux commented May 2, 2024

@andrewkolos Thanks for your insight. I’m far from having a full understanding of asset loading, shaders and flutter tools inner workings. That makes it hard to draw conclusions on this area.
I will close this PR for the moment and I will try to come up with a narrowed investigation.

For context, here is what led me to this PR:

static void initializeShader() {
if (!_initCalled) {
ui.FragmentProgram.fromAsset('shaders/ink_sparkle.frag').then(
(ui.FragmentProgram program) {
_program = program;
},
);
_initCalled = true;
}
}

I will try to get a better understanding of how the shader is supposed to be loaded on web tests and I will try to narrow it down to make progress.

@bleroux bleroux closed this May 2, 2024
@bleroux
Copy link
Contributor Author

bleroux commented May 2, 2024

Just found #31207 which is a great starting point to understand how assets are loaded in non-web tests (it mocks flutter/assets channel to bypass the engine).

@andrewkolos
Copy link
Contributor

Thanks for the recap!

I’m far from having a full understanding of asset loading, shaders and flutter tools inner workings. That makes it hard to draw conclusions on this area.

I work on the tool full-time, and I could still say the same 😆. I would have loved to have a deep dive here and figure all of this out, but I unfortunately do not have the time. If you do figure this out or this turns out to be a higher priority than I thought, I'd be glad to look at this again.

@dvaruas
Copy link

dvaruas commented May 30, 2024

Hello @bleroux I am also facing the same issue where shaders asset is not found in unit tests for flutter app.
Were you able to find anything more during your investigation?

@andrewkolos this really blocks users from using updated Flutter version (since 3.16, and the latest SDK is 3.22). Could the flutter team also take a look into this with priority?

@bleroux
Copy link
Contributor Author

bleroux commented May 31, 2024

Hello @dvaruas!
I have not work on this issue since my last comment. It would help if you can file an issue with a minimal repro. My work was based on #138487 which is a specific case (a shader provided by the framework). Having a proper issue with a simple repro based on a Flutter app that loads a shader would be interesting because it extends the scope of this issue.

this really blocks users from using updated Flutter version (since 3.16, and the latest SDK is 3.22).

Do you mean that it worked before 3.16? (I'm not sure when shaders support was added to Flutter Web)
For Flutter 3.16 and 3.19, there was another issue related to shaders (#141296), it was fixed in flutter/engine#49754 which is included in Flutter 3.22.

@dvaruas
Copy link

dvaruas commented May 31, 2024

Hey @bleroux , thank you for your response. I'll try to create a issue with a minimal example once I isolate it.

Do you mean that it worked before 3.16? (I'm not sure when shaders support was added to Flutter Web)

yes, I was using 3.13.9 previously and everything works fine.
Encountered this issue when I tried to update to 3.16

For now I am removing the --no-test-assets flag to bypass this issue.

@bleroux bleroux deleted the flutter_tools_test_web_makes_ink_sparkle_frag_available branch December 6, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-tool Owned by Flutter Tool team tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants