[shell] Fix engineId not being set after hot restart #174451
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where the engineId was not being preserved during a hot restart. The fix involves storing the engineId from the RunConfiguration within the Engine instance upon execution. This stored ID is then retrieved and applied to the new RunConfiguration during the hot restart process. Additionally, the RunConfiguration::SetEngineId method signature has been updated to accept a std::optional<int64_t>, improving API consistency with the underlying member type and allowing for the engine ID to be explicitly absent.
Member
Author
|
@chinmaygarde, any ideas about the test here? It seems that there are no tests for |
Contributor
|
You could simulate that a
The behavior you are modifying is that updating the run configuration of an engine updates the engine ID. That its all initiated via the service protocol is incidental and more of an integration test concern. A unit-test focused on just Engine::Run should suffice. |
| configuration.SetEntrypoint("providesEngineId"); | ||
| RunEngine(shell.get(), std::move(configuration)); | ||
|
|
||
| ASSERT_EQ(shell->GetEngine()->GetLastEngineId(), 99); |
Contributor
There was a problem hiding this comment.
99 seems like a magic number here. RunConfiguration::SetEngineId to something else and assert that instead?
Also, run it again with a different ID to see that it stick? That way, this test should fail without your patch. As it stands, this test should work with or without your patch right?
Contributor
There was a problem hiding this comment.
It seems that this test is dependent on the previous test setting the ID to 99. Can we make the test more standalone as Chinmay suggests?
Member
Author
There was a problem hiding this comment.
Ouch, that statement should be in different test. Hence the 99 magical value. The test tests for LastEngineId(). There is no LastEngineId without the patch, it was introduced in the PR.
Contributor
HosseinYousefi
left a comment
There was a problem hiding this comment.
As I don't have the domain expertise here, I don't have anything to add in terms of the actual code review. Could you please land this faster @knopp?
@chinmaygarde is it possible to add this patch to a hotfix version? The issue with cronet_http package is blocked on this.
| configuration.SetEntrypoint("providesEngineId"); | ||
| RunEngine(shell.get(), std::move(configuration)); | ||
|
|
||
| ASSERT_EQ(shell->GetEngine()->GetLastEngineId(), 99); |
Contributor
There was a problem hiding this comment.
It seems that this test is dependent on the previous test setting the ID to 99. Can we make the test more standalone as Chinmay suggests?
Member
Author
|
Apologies for the delay. I missed @chinmaygarde feedback and only got pinged now. The assert should be part of another test (once that sets the value 99). I'll move it. |
eb43763 to
db4f0c9
Compare
chinmaygarde
approved these changes
Sep 9, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 10, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 10, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 10, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 11, 2025
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Sep 11, 2025
flutter/flutter@a082096...5a6a1bf 2025-09-11 engine-flutter-autoroll@skia.org Roll Dart SDK from 42045594dbc6 to f7d6a4732ab0 (2 revisions) (flutter/flutter#175222) 2025-09-11 engine-flutter-autoroll@skia.org Roll Skia from 00c8b3f69de9 to ead9277819fc (2 revisions) (flutter/flutter#175221) 2025-09-11 engine-flutter-autoroll@skia.org Roll Skia from 4a8817a48b25 to 00c8b3f69de9 (3 revisions) (flutter/flutter#175213) 2025-09-11 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 10x-JGF5zuuW8ik4K... to 1pTB3J5rn4YYugylf... (flutter/flutter#175210) 2025-09-11 engine-flutter-autoroll@skia.org Roll Dart SDK from 1de2289e49fe to 42045594dbc6 (1 revision) (flutter/flutter#175203) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from cbb0388767d2 to 4a8817a48b25 (4 revisions) (flutter/flutter#175202) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from 55c9d697da52 to cbb0388767d2 (7 revisions) (flutter/flutter#175197) 2025-09-10 engine-flutter-autoroll@skia.org Roll Dart SDK from f446144fb7c9 to 1de2289e49fe (3 revisions) (flutter/flutter#175192) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from c3a3d1e47699 to 55c9d697da52 (4 revisions) (flutter/flutter#175190) 2025-09-10 engine-flutter-autoroll@skia.org Roll Packages from 2d651b2 to 03598e7 (5 revisions) (flutter/flutter#175185) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from 36f3c3fbec19 to c3a3d1e47699 (2 revisions) (flutter/flutter#175181) 2025-09-10 fluttergithubbot@gmail.com Marks Linux plugin_test_android_standard to be unflaky (flutter/flutter#175163) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from 97497ee065e4 to 36f3c3fbec19 (5 revisions) (flutter/flutter#175178) 2025-09-10 fluttergithubbot@gmail.com Marks Windows plugin_test_android_variants to be unflaky (flutter/flutter#175167) 2025-09-10 fluttergithubbot@gmail.com Marks Linux plugin_test_android_variants to be unflaky (flutter/flutter#175162) 2025-09-10 fluttergithubbot@gmail.com Marks Windows plugin_test_android_standard to be unflaky (flutter/flutter#175168) 2025-09-10 fluttergithubbot@gmail.com Marks Mac plugin_test_android_standard to be unflaky (flutter/flutter#175166) 2025-09-10 fluttergithubbot@gmail.com Marks Mac plugin_test_android_variants to be unflaky (flutter/flutter#175165) 2025-09-10 matej.knopp@gmail.com [shell] Fix engineId not being set after hot restart (flutter/flutter#174451) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from 6acb8b29b60e to 97497ee065e4 (1 revision) (flutter/flutter#175152) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from 127786500ad0 to 6acb8b29b60e (1 revision) (flutter/flutter#175148) 2025-09-10 engine-flutter-autoroll@skia.org Roll Skia from 04513dfdf517 to 127786500ad0 (2 revisions) (flutter/flutter#175145) 2025-09-09 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from m7Qmvj5wtfPlMA8i8... to 10x-JGF5zuuW8ik4K... (flutter/flutter#175140) 2025-09-09 engine-flutter-autoroll@skia.org Roll Skia from 416b3b42ece2 to 04513dfdf517 (5 revisions) (flutter/flutter#175137) 2025-09-09 engine-flutter-autoroll@skia.org Roll Skia from 19ba56dde579 to 416b3b42ece2 (1 revision) (flutter/flutter#175134) 2025-09-09 victorsanniay@gmail.com Adjust default CupertinoCheckbox size on desktop (flutter/flutter#172502) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutteractionsbot
pushed a commit
to flutteractionsbot/flutter
that referenced
this pull request
Sep 12, 2025
Fixes flutter#173474 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
flutteractionsbot
pushed a commit
to flutteractionsbot/flutter
that referenced
this pull request
Sep 12, 2025
Fixes flutter#173474 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
mboetger
pushed a commit
to mboetger/flutter
that referenced
this pull request
Sep 18, 2025
Fixes flutter#173474 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Jaineel-Mamtora
pushed a commit
to Jaineel-Mamtora/flutter_forked
that referenced
this pull request
Sep 24, 2025
Fixes flutter#173474 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Nov 12, 2025
lucaantonelli
pushed a commit
to lucaantonelli/flutter
that referenced
this pull request
Nov 21, 2025
Fixes flutter#173474 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #173474
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.