Fix custom-device default forwardPortSuccessRegex by mbriand · Pull Request #97719 · flutter/flutter

mbriand · 2022-02-03T16:55:11Z

This PR tries to make port forwarding verification on custom devices more reliable, by not relying on behavior that may change across different systems.

Previously we were looking for Linux word in target connection banner. Now we ask the remote to print a fixed string ('Port forwarding success`) and look for this given string to ensure the connection was successful.

Fix #97641

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.
All existing and new tests are passing.

flutter-dashboard · 2022-02-03T16:55:14Z

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 Hixie on the #hackers channel in Chat.

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.

christopherfujino

I think this would need a test before we can merge.

mbriand · 2022-02-23T17:07:21Z

Sorry, I was not very active on this PR.

So yes, I would be happy to add some tests here. Actually my modification made some tests to fail, but these tests were removed in commit 87fe3fe (#98299), as they were failing for another issue, that I believe is actually a bug.

I managed to fix this bug and make the test pass, would you mind if I import back these tests ? I'm pushing these modifications, just to detail what I am talking about.

christopherfujino · 2022-02-24T19:51:43Z

Sorry, I was not very active on this PR.

So yes, I would be happy to add some tests here. Actually my modification made some tests to fail, but these tests were removed in commit 87fe3fe (#98299), as they were failing for another issue, that I believe is actually a bug.

I managed to fix this bug and make the test pass, would you mind if I import back these tests ? I'm pushing these modifications, just to detail what I am talking about.

Oh nice! This is great.

christopherfujino · 2022-02-24T19:52:06Z

The analysis failure was fixed upstream, a rebase should fix

christopherfujino

LGTM

Tests were removed in flutter#98299

Having an empty list as postBuildCommand value will trigger an error on JSON parsing: make sure to save a null instead.

mbriand · 2022-02-25T09:30:20Z

Sorry, I still have an issue with the "Linux analyze" test. It complains that I have to "Avoid redundant argument values", but if I remove the postBuildCommand parameter, it complains that "The parameter 'postBuildCommand' is required".

   info â�¢ Avoid redundant argument values â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:557:33 â�¢ avoid_redundant_argument_values
   info â�¢ Avoid redundant argument values â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:647:33 â�¢ avoid_redundant_argument_values
   info â�¢ Avoid redundant argument values â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:737:33 â�¢ avoid_redundant_argument_values
   info â�¢ Avoid redundant argument values â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:831:33 â�¢ avoid_redundant_argument_values
   info â�¢ Avoid redundant argument values â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:911:33 â�¢ avoid_redundant_argument_values
   info â�¢ Avoid redundant argument values â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:1236:33 â�¢ avoid_redundant_argument_values

warning â�¢ The parameter 'postBuildCommand' is required â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:546:13 â�¢ missing_required_param
warning â�¢ The parameter 'postBuildCommand' is required â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:635:13 â�¢ missing_required_param
warning â�¢ The parameter 'postBuildCommand' is required â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:723:13 â�¢ missing_required_param
warning â�¢ The parameter 'postBuildCommand' is required â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:817:19 â�¢ missing_required_param
warning â�¢ The parameter 'postBuildCommand' is required â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:896:13 â�¢ missing_required_param
warning â�¢ The parameter 'postBuildCommand' is required â�¢ packages/flutter_tools/test/commands.shard/hermetic/custom_devices_test.dart:1219:13 â�¢ missing_required_param

So I explicitly added // ignore: avoid_redundant_argument_values. Is that the right way to do it ?

fluttergithubbot · 2022-03-03T22:46:13Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

christopherfujino · 2022-03-03T23:52:39Z

@Jasguerrero can you review this one too?

Jasguerrero

LGTM

…utter#97719)

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 3, 2022

christopherfujino reviewed Feb 17, 2022

View reviewed changes

mbriand force-pushed the mbria/fix_portforward_regex branch from 1796496 to 840fe15 Compare February 23, 2022 17:08

christopherfujino approved these changes Feb 24, 2022

View reviewed changes

mbriand added 4 commits February 25, 2022 08:57

Add back deleted flutter devices tests

f09a8b9

Tests were removed in flutter#98299

Fix custom-device postBuildCommand in JSON

cb70b40

Having an empty list as postBuildCommand value will trigger an error on JSON parsing: make sure to save a null instead.

Fix custom-device default forwardPortSuccessRegex

638b86a

Fix custom-devices tests for forwardPort

0cb9761

mbriand force-pushed the mbria/fix_portforward_regex branch from 9b80bd4 to 0cb9761 Compare February 25, 2022 07:58

fixup! Fix custom-device postBuildCommand in JSON

394dfce

mbriand force-pushed the mbria/fix_portforward_regex branch from c6402dc to 394dfce Compare February 25, 2022 08:55

christopherfujino added the waiting for tree to go green label Mar 3, 2022

fluttergithubbot removed the waiting for tree to go green label Mar 3, 2022

christopherfujino requested a review from Jasguerrero March 3, 2022 23:52

Jasguerrero approved these changes Mar 4, 2022

View reviewed changes

christopherfujino added the waiting for tree to go green label Mar 4, 2022

fluttergithubbot merged commit 020bf31 into flutter:master Mar 4, 2022

engine-flutter-autoroll mentioned this pull request Mar 4, 2022

Roll Flutter from 38dbbb17f8ce to 020bf31c990e (46 revisions) flutter/packages#1053

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

e39bfd4

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 4, 2022

Roll Flutter from 38dbbb17f8ce to 852bfe2a73e2 (47 revisions) flutter/packages#1054

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

b0c2040

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 4, 2022

Roll Flutter from 38dbbb17f8ce to 09c7eb1b88cf (48 revisions) flutter/packages#1055

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 4, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

9b58d8a

…utter#97719)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

cda1035

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 436c35005626 (75 revisions) flutter/packages#1099

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

5a9bb0a

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 5533b90fed75 (76 revisions) flutter/packages#1100

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

f8cfa1a

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to f307db272e3c (77 revisions) flutter/packages#1101

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

dfede12

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 252022eafdbc (78 revisions) flutter/packages#1102

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

9bbff04

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 67f25caf727c (79 revisions) flutter/packages#1104

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

6a731da

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to b02150781f04 (80 revisions) flutter/packages#1105

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

a23d77f

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to fbbf3ec4712b (81 revisions) flutter/packages#1106

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

c23b3bb

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 8f5b5492bed2 (82 revisions) flutter/packages#1107

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

3274c7a

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to a491a81f7873 (83 revisions) flutter/packages#1108

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

e05cc27

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 66826e8fefb9 (84 revisions) flutter/packages#1109

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

57fe9b5

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 4f7d7b56191d (85 revisions) flutter/packages#1110

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

0afee2e

…utter#97719)

engine-flutter-autoroll mentioned this pull request Mar 7, 2022

Roll Flutter from 38dbbb17f8ce to 4c591bc755c4 (86 revisions) flutter/packages#1111

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2022

020bf31 Fix custom-device default forwardPortSuccessRegex (flutter/fl…

60cf8b6

…utter#97719)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix custom-device default forwardPortSuccessRegex#97719

Fix custom-device default forwardPortSuccessRegex#97719
fluttergithubbot merged 5 commits intoflutter:masterfrom
mbriand:mbria/fix_portforward_regex

mbriand commented Feb 3, 2022

Uh oh!

flutter-dashboard bot commented Feb 3, 2022

Uh oh!

christopherfujino left a comment

Uh oh!

mbriand commented Feb 23, 2022

Uh oh!

christopherfujino commented Feb 24, 2022

Uh oh!

christopherfujino commented Feb 24, 2022

Uh oh!

christopherfujino left a comment

Uh oh!

mbriand commented Feb 25, 2022

Uh oh!

fluttergithubbot commented Mar 3, 2022

Uh oh!

christopherfujino commented Mar 3, 2022

Uh oh!

Jasguerrero left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mbriand commented Feb 3, 2022

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 3, 2022

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

mbriand commented Feb 23, 2022

Uh oh!

christopherfujino commented Feb 24, 2022

Uh oh!

christopherfujino commented Feb 24, 2022

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

mbriand commented Feb 25, 2022

Uh oh!

fluttergithubbot commented Mar 3, 2022

Uh oh!

christopherfujino commented Mar 3, 2022

Uh oh!

Jasguerrero left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants