[iOS] fix hot restart with native assets by knopp · Pull Request #148752 · flutter/flutter

knopp · 2024-05-21T08:38:11Z

Adds support for running the hot restart and reload integration test on iOS simulator.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

knopp · 2024-05-21T12:03:49Z

@dcharkes, I think this should work, but the tree is currently broken and the integration tests are currently failing for unrelated reason.

knopp · 2024-05-21T17:08:46Z

@dcharkes, it seems that the hot reload test (with the fix) passes successfully on simulator on CI:

21:26 +72 ~1: test/integration.shard/isolated/native_assets_test.dart: flutter run hot reload and hot restart with native assets 873FC224-4044-471E-9CFA-9739B82F3105 debug

dcharkes

Great!

Before merging, could you check the existing device lab test? That already spin up a simulator.

dcharkes · 2024-05-22T16:15:15Z

We have all this logic already in the device lab tests.

https://github.com/flutter/flutter/blob/e6fa8655819e9b92740911b49e8215efc6e52dab/dev/devicelab/bin/tasks/native_assets_ios_simulator.dart

$ cd <...>/flutter/dev/devicelab && dart bin/test_runner.dart test -t native_assets_ios_simulator

Would you be able to check why the existing hot integration test was not failing on the ios simulator?

(I don't feel strongly opposed to adding the logic also here, but since we already have it in the device lab tests it feels a bit like unnecessary code duplication.)

I think the problem is that when hot restart happens this is in the logs

Performing hot restart... Restarted application in 325ms. ══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════ The following ArgumentError was thrown building _FocusInheritedScope:

The test only checks for Restarted application, which is there, so it assumes it passed, the exception is printed after. I think this is handled bit differently in the integration tests that's why those catch the issue.

This is present in the integration test

expect(stdout, isNot(contains('EXCEPTION CAUGHT BY WIDGETS LIBRARY')));

Probably should be added to device lab tool

But even if I detect the exception and return TaskResult.failure the task still exits with success for some reason.

@dcharkes, this should fix the device lab test: eb6b04c

Should I remove the simulator from integration tests?

I wouldn't mind having it there, it's easier to run locally than the devicelab test.

However, I believe we shouldn't be using infinite resources on CI. And the test coverage should be similar between the integration test with the simulator and the devicelab test with the simulator.

cc @jmagman @vashworth What's our policy on how much testing resources we use? And what's the policy on where simulator tests should run? (Integration suite vs device lab. I'm added the simulator to the device lab based on existing device lab tests that run the simulator.)

Thanks for diving into this @knopp! ❤️

The 4/4 shard already takes long time to complete (in fact it did timeout in previous run and I had to restart it). I think maybe it's a good idea to remove it.

Yeah that test runs long already. We don't want to introduce flakes due to timeouts. Let's remove it from this PR.

That means that this PR will just be a test fix and a fix 👍

dcharkes · 2024-05-23T12:40:03Z

Please also fix dev/devicelab/bin/tasks/native_assets_ios.dart

That one seems fine? I looked at it and it seems to propagate the result of createNativeAssetsTest correctly.

dcharkes

LGTM with simulator removed again from integration test.

knopp · 2024-05-23T14:29:17Z

@dcharkes flutter run hot reload and hot restart with native assets macos debug failed:

34:35 +84 ~1 -2: test/integration.shard/isolated/native_assets_test.dart: flutter run hot reload and hot restart with native assets macos debug [E]
  Expected: not contains 'Invalid argument(s): Couldn\'t resolve native function \'sum\''
    Actual: 'warning: Run script build phase \'Run Script\' will be run during every build because it does not specify any outputs. To address this warning, either add output dependencies to the script phase, or configure it to run in every build by unchecking "Based on dependency analysis" in the script phase. (in target \'Flutter Assemble\' from project \'Runner\')\n'
              'Performing hot reload...                                        \n'
              'The Flutter DevTools debugger and profiler on macOS is available at: http://127.0.0.1:9106?uri=http://127.0.0.1:53308/xYohxuqAUcQ=/\n'
              'Reloaded 0 libraries in 520ms (compile: 8 ms, reload: 0 ms, reassemble: 492 ms).\n'
              '\n'
              'Performing hot restart...                                       \n'
              'Restarted application in 5,343ms.\n'
              '\n'
              'Performing hot reload...                                        \n'
              '\n'
              '══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════\n'
              'The following ArgumentError was thrown building _FocusInheritedScope:\n'
              'Invalid argument(s): Couldn\'t resolve native function \'sum\' in\n'
              '\'package:package_with_native_assets/package_with_native_assets_bindings_generated.dart\' : Failed to\n'
              'load dynamic library \'package_with_native_assets1.framework/package_with_native_assets1\': Failed to\n'
              'load dynamic library \'package_with_native_assets1.framework/package_with_native_assets1\':\n'
              'dlopen(package_with_native_assets1.framework/package_with_native_assets1, 0x0001): tried:\n'
              '\'package_with_native_assets1.framework/package_with_native_assets1\' (no such file),\n'
              '\'/System/Volumes/Preboot/Cryptexes/OSpackage_with_native_assets1.framework/package_with_native_assets1\'\n'
              '(no such file),\n'

I don't think it is related to this PR. I'm going to restart the test to see if it passes, but I think it might be flaky?

This reverts commit 24d81275222cc480db740cb91d4204a009faeaed.

This reverts commit fdbb5637ca8ef46ae86ba018ac9a413d8fe087f7.

This reverts commit 55fb3865e8a150553e1c129bfb20fe1d32ce27cc.

This reverts commit b69c39c536711cc8563f2f27d0c2539b72b17fd4.

)

github-actions Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 21, 2024

knopp mentioned this pull request May 21, 2024

[iOS] Hot restart fails with native assets #148687

Closed

knopp force-pushed the native_asset_integration_test_simulator branch 2 times, most recently from 93cd0b9 to 297c0eb Compare May 21, 2024 09:53

knopp changed the title ~~Run native assets integration tests on iOS simulator~~ [iOS] fix hot reload with native assets May 21, 2024

knopp changed the title ~~[iOS] fix hot reload with native assets~~ [iOS] fix hot restart with native assets May 21, 2024

knopp force-pushed the native_asset_integration_test_simulator branch from 29571ba to 8cef4c2 Compare May 21, 2024 16:21

knopp requested a review from dcharkes May 21, 2024 17:08

knopp force-pushed the native_asset_integration_test_simulator branch from 8cef4c2 to 6552cec Compare May 22, 2024 11:24

dcharkes requested changes May 22, 2024

View reviewed changes

knopp force-pushed the native_asset_integration_test_simulator branch 2 times, most recently from 343cf29 to eb6b04c Compare May 23, 2024 08:32

dcharkes reviewed May 23, 2024

View reviewed changes

dcharkes approved these changes May 23, 2024

View reviewed changes

knopp added 10 commits May 23, 2024 16:29

Run native assets integration tests on iOS simulator

2ab293e

type annotations

3f5d081

create and destroy simulator for the test

df2d19c

tweaks

17f25b7

Fix assets with sameId producing multiple paths

c858195

Ensure native asset device lab test fails on exception

8082516

Revert "tweaks"

2b5a99b

This reverts commit 24d81275222cc480db740cb91d4204a009faeaed.

Revert "create and destroy simulator for the test"

e0e27a7

This reverts commit fdbb5637ca8ef46ae86ba018ac9a413d8fe087f7.

Revert "type annotations"

2a5928a

This reverts commit 55fb3865e8a150553e1c129bfb20fe1d32ce27cc.

Revert "Run native assets integration tests on iOS simulator"

8eca905

This reverts commit b69c39c536711cc8563f2f27d0c2539b72b17fd4.

knopp force-pushed the native_asset_integration_test_simulator branch from edf78f0 to 8eca905 Compare May 23, 2024 14:29

knopp merged commit 02d5286 into flutter:master May 23, 2024

knopp deleted the native_asset_integration_test_simulator branch May 23, 2024 16:30

engine-flutter-autoroll mentioned this pull request May 23, 2024

Roll Flutter from 73bf206f35e7 to 02d5286e02e4 (23 revisions) flutter/packages#6785

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 23, 2024

02d5286 [iOS] fix hot restart with native assets (flutter/flutter#148752

27ef106

)

engine-flutter-autoroll mentioned this pull request May 23, 2024

Roll Flutter from 73bf206f35e7 to 8d955cdd644e (24 revisions) flutter/packages#6786

Merged

Conversation

knopp commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

knopp commented May 21, 2024

Uh oh!

knopp commented May 21, 2024

Uh oh!

dcharkes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcharkes left a comment

Choose a reason for hiding this comment

Uh oh!

knopp commented May 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

knopp commented May 21, 2024 •

edited

Loading