Manual roll Flutter from 911aa7547ed7 to 043b71954ce7 #8693
Conversation
stuartmorgan
requested review from
bparrishMines,
camsim99,
matanlurey,
gmackall,
reidbaker and
tarrinneal
as code owners
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
.ci/flutter_master.version
Outdated
There was a problem hiding this comment.
Where can I read more about how this file works in the package repo
There was a problem hiding this comment.
I don't know that it's specifically documented anywhere, but its used in ci.yaml (example), and it's rolled by the auto-roller. The generic packages recipe uses that ci.yaml file to control the Flutter version that is checked out.
| flutterEngine.getPlugins().add(new TestPlugin()); | ||
| flutterEngine.getPlugins().add(new IntegrationTestPlugin()); |
There was a problem hiding this comment.
Why do we not need the integration test plugin? was it dead code?
There was a problem hiding this comment.
This is a speculative change, but the fact that this wasn't calling super seems suspicious. Normally plugins are registered by the generated registrant.
This wasn't compiling because it's unconditionally using the integration test plugin, but that plugin is a dev dependency. If this doesn't work, I'll need to make this example app have a non-dev dependency on it instead, but that feels wrong.
| @@ -54,7 +54,7 @@ android { | |||
| testImplementation 'org.jetbrains.kotlin:kotlin-test' | |||
| testImplementation "org.mockito.kotlin:mockito-kotlin:5.4.0" | |||
| testImplementation 'org.mockito:mockito-inline:5.1.0' | |||
| testImplementation 'androidx.test:core:1.3.0' | |||
There was a problem hiding this comment.
FYI I filed flutter/flutter#164025 to track making these updates easier.
|
Looks like some tests are failing now; I'll investigate further tomorrow. |
|
Hm, it looks like integration tests are failing pretty much across the board. @matanlurey Did you test with your changes to see if the instructions here and here are still correct? I'm wondering if having a dev dependency on |
Neither of those two steps should have been impacted. The scenario I could imagine breaking is if the result of To test that theory you could move the dependency from Otherwise I would slap the config opt-out and assign me a P1, and I'll look at it tomorrow morning, and rollback the flag if there isn't a revert. Bonus points if there is a local repro. |
That's definitely not the case; this is what we are deploying: packages/script/tool/lib/src/firebase_test_lab_command.dart Lines 285 to 288 in c44c228
Running now with that change for the |
#8692 with fixes for indirect effects of
--explicit-package-dependencies