Add current version to the upgrade message of the Flutter tool by karamage · Pull Request #68421 · flutter/flutter

karamage · 2020-10-18T03:53:13Z

Description

Add current version to the upgrade message of the Flutter tool.

I'm a first-time contributor.
There are many things I don't understand, so please let me know.
Please point out any mistakes.
Thank you for reviewing it.

Upgrading Flutter to 1.22.1 from 1.22.0 in C:\Programming\Flutter\sdk\flutter...                                                            Checking Dart SDK version...                                                                                            Downloading Dart SDK from Flutter engine c8e3b9485386425213e2973126d6f57e7ed83c54...                                    Unzipping Dart SDK...
Building flutter tool...
Running pub upgrade...

Upgrading engine...

Related Issues

Add current version to the upgrade message of the Flutter tool. #63023

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A change in behavior with no test covering it
will likely get reverted accidentally sooner or later. PRs must include tests for all changed/updated/fixed behaviors. See Test Coverage.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-10-18T03:53:16Z

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.

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

google-cla · 2020-10-18T03:53:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

jonahwilliams · 2020-10-22T20:44:39Z

@karamage can you sign the CLA?

karamage · 2020-10-22T23:40:05Z

@jonahwilliams
OK.
I signed the CLA.

karamage · 2020-10-22T23:41:22Z

@googlebot I signed it!

jonahwilliams

Seems like a nice change! If you can add a test case I'd be happy to land this

jonahwilliams · 2020-10-23T04:44:02Z

packages/flutter_tools/lib/src/commands/upgrade.dart

We don't currently have any tests for what gets printed during the upgrade. The closest example is one that checks that a message is printed if nothing is upgraded:

https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart#L92

I would see if you adapt that to add coverage for this.

Thanks for reviewing it for you.
I agree with you about adding a test.
I'll TRY to do it.

I'm sorry.
I tried, but I can't add the test.
I want to check the standard output, please tell me how to do it.

Reference.

expect(testLogger.statusText, contains('Flutter is already up to date'));

I ran the test and the testLogger.statusText was empty.

Please let me know.

@karamage check if the status text you want to display is actually being displayed, like these lines in a similar test:

flutter/packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart

Lines 115 to 117 in fb508db

expect(testLogger.statusText, contains('A new version of Flutter is available'));

expect(testLogger.statusText, contains(fakeCommandRunner.remoteRevision));

expect(testLogger.statusText, contains(revision));

I think you should take this (similar)test as a reference to create your new test:

flutter/packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart

Lines 310 to 314 in fb508db

testUsingContext("Doesn't throw on known tag, dev branch, no force", () async {

final Future<FlutterCommandResult> result = fakeCommandRunner.runCommand(

force: false,

continueFlow: false,

testFlow: false,

@royarg02 Thank you. I'll try.

Try this out for a bit and let me know if you make any progress. I will take a look at this PR again tomorrow and make some suggestions to get your test working correctly

@karamage I think you need to use realCommandRunner in this case, as fakeCommandRunner.upgradeChannel doesn't do anything:

flutter/packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart

Line 401 in fb508db

Future<void> upgradeChannel(FlutterVersion flutterVersion) async {}

It also explains you getting Actual: '' in your test.

Thank you. I appreciate it.

Okay, I understand.

I will try to write the test code using realCommandRunner.

@royarg02 @jonahwilliams

I've added the test code.

I would like you to review it.

Thank you for showing me how to test it.

The test was a success.

../../bin/dart pub run test test/commands.shard/permeable/upgrade_test.dart 00:04 +15: All tests passed!

jonahwilliams · 2020-10-23T04:44:44Z

packages/flutter_tools/lib/src/commands/upgrade.dart

I would leave this message in upgradeChannel, and instead pass the upstreamRevision revision in as an argument.

Understood. I will modify it to pass in arguments.

jonahwilliams · 2020-10-23T04:45:11Z

packages/flutter_tools/lib/src/commands/upgrade.dart

For just a single identifier you don't need the{}:

Suggested change

globals.printStatus('Upgrading Flutter to ${upstreamRevision} from ${flutterVersion.frameworkRevision} ($workingDirectory...)');

globals.printStatus('Upgrading Flutter to $upstreamRevision from ${flutterVersion.frameworkRevision} ($workingDirectory...)');

I'll fix it.

packages/flutter_tools/lib/src/commands/upgrade.dart

google-cla · 2020-10-28T14:08:57Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

karamage · 2020-10-28T14:13:37Z

@googlebot I consent.

google-cla · 2020-10-28T14:13:43Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

royarg02 · 2020-10-28T14:14:24Z

@googlebot I consent.

jonahwilliams

LGTM!

I'd also like @christopherfujino to double check too. Thank you @royarg02 for the help as well

royarg02 · 2020-10-29T16:16:03Z

packages/flutter_tools/lib/src/commands/upgrade.dart

I just realized that flutterVersion.frameworkRevision is probably not what the PR was intending to use:

Take a look at these lines of code:

flutter/packages/flutter_tools/lib/src/version.dart

Lines 170 to 183 in 13896b3

String toString() {

final String versionText = frameworkVersion == 'unknown' ? '' : ' $frameworkVersion';

final String flutterText = 'Flutter$versionText • channel $channel • ${repositoryUrl ?? 'unknown source'}';

final String frameworkText = 'Framework • revision $frameworkRevisionShort ($frameworkAge) • $frameworkCommitDate';

final String engineText = 'Engine • revision $engineRevisionShort';

final String toolsText = 'Tools • Dart $dartSdkVersion';

// Flutter 1.10.2-pre.69 • channel master • https://github.com/flutter/flutter.git

// Framework • revision 340c158f32 (84 minutes ago) • 2018-10-26 11:27:22 -0400

// Engine • revision 9c46333e14

// Tools • Dart 2.1.0 (build 2.1.0-dev.8.0 bf26f760b1)

return '$flutterText\n$frameworkText\n$engineText\n$toolsText';

}

My understanding is that flutterVersion.frameworkVersion should be used, but is there anyway the corresponding version of the upstreamRevision could be obtained?

Thats a good point, it is probably printing out the commit hash right now. We should know what the tag we're checking out is

This might be something of interest in that case:

flutter/packages/flutter_tools/lib/src/version.dart

Line 77 in 13896b3

_frameworkVersion = gitTagVersion.frameworkVersionFor(_frameworkRevision);

Thats a good point, it is probably printing out the commit hash right now. We should know what the tag we're checking out is

@jonahwilliams You're right. #68866 was merged yesterday which uses flutterVersion.frameworkRevision:

$ flutter upgrade --verify-only A new version of Flutter is available on channel master The latest revision: 18341efa2fb8c3570ca32e71d1a9da74e8b2cbf5 Your current version: 8b9e9680d24ffd4b3dbfa216f5f64a0490088f45 To upgrade now, run "flutter upgrade".

@royarg02 @jonahwilliams Thanks for reviewing it for you.
I understand that using flutterVersion.frameworkRevision is a mistake.
Thank you.

I will modify it to use frameworkVersion.

I have modified it to use frameworkVersion.
I also rewrote the test and it was successful!
Please give you a review.

I'm taking a look now.

out of date

christopherfujino · 2020-10-30T20:39:08Z

packages/flutter_tools/lib/src/commands/upgrade.dart

Unfortunately, I don't believe this is right either. The .frameworkVersionFor() method is confusingly named. It does not return the correct version for the input revision you input, but rather the GitTagVersion that it is a method for. And on line 157, I believe that gitTagVersion is the current HEAD, or the version that the tool is currently at.

Please see my comments at @royarg02's PR for more context: https://github.com/flutter/flutter/pull/69420/files#r515275570. It seems like are both trying to hook into functionality that we don't yet support (namely, creating a GitTagVersion object for a commit that ISN'T HEAD). It would be great if the two of you could coordinate implementing this.

@karamage I've updated #69420 as per suggestions from @christopherfujino. Please have a look at it(I've not updated the tests yet).

If you need any clarifications about my approach, or have a better idea on implementing the feature, we can discuss on the chat.

@royarg02 @christopherfujino Thanks for your suggestion.
I'll look at #69420.

I am trying to understand your approach.

royarg02 · 2020-11-20T04:45:59Z

@karamage #69420 has landed. You can work on this PR now.

karamage · 2020-11-20T07:29:49Z

@royarg02 Thank you. I will work on this PR.

karamage · 2020-11-21T02:47:09Z

I went back to the changes because of the conflicts.

I will continue to change the PR from here.

It's going to take some time to understand the changes.

karamage · 2020-11-22T07:48:10Z

I saw #69420. It was very informative. Thank you.

I wrote the code for this PR, but I'm not sure if it's good enough.

The upgradeChannel method is gone, so I made a change to the recordState method.

If I'm wrong, please let me know.

I wrote the test code and ran the test.
The test was successful.

Thank you for reading.

royarg02 · 2020-11-22T09:18:27Z

packages/flutter_tools/lib/src/commands/upgrade.dart


-  void recordState(FlutterVersion flutterVersion) {
+  void recordState(FlutterVersion flutterVersion, FlutterVersion upstreamVersion) {
+    globals.printStatus('Upgrading Flutter to ${upstreamVersion.frameworkVersion} from ${flutterVersion.frameworkVersion} in $workingDirectory...');


It might be better if you directly put this line in runCommandFirstHalf after calling recordState.

royarg02 · 2020-11-22T09:18:35Z

packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart

+      const String upstreamVersion = '4.5.6';
+
+      when(flutterVersion.frameworkRevision).thenReturn(revision);
+      when(flutterVersion.frameworkRevisionShort).thenReturn(revision);


I don't think you'll need flutterVersion.frameworkRevisionShort in your test.
Same for latestVersion.frameworkRevisionShort below.

royarg02 · 2020-11-22T09:21:21Z

packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart

+        ),
+      );
+      realCommandRunner.recordState(flutterVersion, latestVersion);
+      expect(testLogger.statusText, contains('Upgrading Flutter to ${latestVersion.frameworkVersion} from ${flutterVersion.frameworkVersion} in ${realCommandRunner.workingDirectory}'));


Tests should be matched against hardcoded strings.
See #69420 (comment) for context.

karamage · 2020-11-23T02:25:56Z

@royarg02 Your suggestions were very helpful. Thank you.
I have modified the code according to your suggestion.
I would appreciate it if you could take a look at the code again.

royarg02 · 2020-11-23T04:52:36Z

packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart

+        ]),
+      ]);
+
+      await realCommandRunner.runCommandFirstHalf(force: true, gitTagVersion: gitTagVersion, flutterVersion: flutterVersion, testFlow: true, verifyOnly: false);


Since the message you are expecting is in runCommandFirstHalf, it would be preferable to run fakeCommandRunner.runCommand and await for FlutterCommandResult.success instead of adding commands explicitly to processManager, just like other tests.

Also, please format the call as following:

final Future<FlutterCommandResult> result = fakeCommandRunner.runCommand( force: false, continueFlow: false, testFlow: false, gitTagVersion: gitTagVersion, flutterVersion: flutterVersion, verifyOnly: true, );

royarg02 · 2020-11-23T04:52:41Z

packages/flutter_tools/test/commands.shard/permeable/upgrade_test.dart

+      ]);
+
+      await realCommandRunner.runCommandFirstHalf(force: true, gitTagVersion: gitTagVersion, flutterVersion: flutterVersion, testFlow: true, verifyOnly: false);
+      expect(testLogger.statusText, contains('Upgrading Flutter to 4.5.6 from 1.2.3 in null'));


Could you change null to something meaningful? I think using fakeCommandRunner.workingDirectory may fix this.

karamage · 2020-11-23T05:51:54Z

@royarg02 The improvements you have pointed out have been very helpful.
Thanks to that, I now understand how to use the fakeCommandRunner.

I have fixed the test code, could you take a look at it?
Thank you, I've fixed it and would appreciate it if you could take a look.

royarg02 · 2020-11-23T06:57:04Z

Everything looks good, let's see how the maintainers respond.

You may want to update the original PR comment.

christopherfujino

LGTM! Thanks so much @karamage for making this change and @royarg02 for helping and doing code review.

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 18, 2020

google-cla bot added the cla: no label Oct 18, 2020

google-cla bot added cla: yes and removed cla: no labels Oct 22, 2020

jonahwilliams self-requested a review October 23, 2020 04:42

jonahwilliams reviewed Oct 23, 2020

View reviewed changes

wf227 mentioned this pull request Oct 23, 2020

[web] Adding a icon to the AppBar via Text.rich() throws exception with ..._USE_SKIA=true #68879

Closed

royarg02 reviewed Oct 28, 2020

View reviewed changes

packages/flutter_tools/lib/src/commands/upgrade.dart Outdated Show resolved Hide resolved

karamage force-pushed the show_current_version_on_upgrade branch from ba1ddb5 to 1b842c5 Compare October 28, 2020 14:08

google-cla bot added cla: no and removed cla: yes labels Oct 28, 2020

google-cla bot added cla: yes and removed cla: no labels Oct 28, 2020

jonahwilliams previously approved these changes Oct 29, 2020

View reviewed changes

royarg02 reviewed Oct 29, 2020

View reviewed changes

christopherfujino requested a review from jonahwilliams October 30, 2020 20:13

jonahwilliams requested review from jonahwilliams and removed request for jonahwilliams October 30, 2020 20:14

christopherfujino changed the title ~~Add current version to the upgrade message of the Flutter tool. #63023~~ Add current version to the upgrade message of the Flutter tool Oct 30, 2020

christopherfujino reviewed Oct 30, 2020

View reviewed changes

karamage mentioned this pull request Nov 10, 2020

[flutter_tools] Make flutter upgrade --verify-only display framework version differences instead of commit hashes #69420

Merged

9 tasks

karamage added 3 commits November 21, 2020 10:52

add test for upgradeChannel. flutter#63023

049d524

Fix to use frameworkVersion flutter#63023

eab0666

tweak.

5bba3db

karamage force-pushed the show_current_version_on_upgrade branch from 459da7e to 5bba3db Compare November 21, 2020 02:37

karamage added 2 commits November 22, 2020 16:31

Show current version to the upgrade message.

8624d88

Remove unnecessary parts in test code

afc2e18

royarg02 reviewed Nov 22, 2020

View reviewed changes

fix the points pointed out in the review

348656f

karamage force-pushed the show_current_version_on_upgrade branch from a313aa9 to 348656f Compare November 23, 2020 02:20

royarg02 reviewed Nov 23, 2020

View reviewed changes

improve test code

189ed10

karamage requested a review from christopherfujino November 23, 2020 07:19

christopherfujino approved these changes Nov 23, 2020

View reviewed changes

christopherfujino merged commit accedc6 into flutter:master Nov 23, 2020

jmagman mentioned this pull request Nov 24, 2020

Add current version to the upgrade message of the Flutter tool. #63023

Closed

	expect(testLogger.statusText, contains('A new version of Flutter is available'));
	expect(testLogger.statusText, contains(fakeCommandRunner.remoteRevision));
	expect(testLogger.statusText, contains(revision));

	testUsingContext("Doesn't throw on known tag, dev branch, no force", () async {
	final Future<FlutterCommandResult> result = fakeCommandRunner.runCommand(
	force: false,
	continueFlow: false,
	testFlow: false,

	globals.printStatus('Upgrading Flutter to ${upstreamRevision} from ${flutterVersion.frameworkRevision} ($workingDirectory...)');
	globals.printStatus('Upgrading Flutter to $upstreamRevision from ${flutterVersion.frameworkRevision} ($workingDirectory...)');

	String toString() {
	final String versionText = frameworkVersion == 'unknown' ? '' : ' $frameworkVersion';
	final String flutterText = 'Flutter$versionText • channel $channel • ${repositoryUrl ?? 'unknown source'}';
	final String frameworkText = 'Framework • revision $frameworkRevisionShort ($frameworkAge) • $frameworkCommitDate';
	final String engineText = 'Engine • revision $engineRevisionShort';
	final String toolsText = 'Tools • Dart $dartSdkVersion';

	// Flutter 1.10.2-pre.69 • channel master • https://github.com/flutter/flutter.git
	// Framework • revision 340c158f32 (84 minutes ago) • 2018-10-26 11:27:22 -0400
	// Engine • revision 9c46333e14
	// Tools • Dart 2.1.0 (build 2.1.0-dev.8.0 bf26f760b1)

	return '$flutterText\n$frameworkText\n$engineText\n$toolsText';
	}

Conversation

karamage commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Oct 18, 2020

Uh oh!

google-cla bot commented Oct 18, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

jonahwilliams commented Oct 22, 2020

Uh oh!

karamage commented Oct 22, 2020

Uh oh!

karamage commented Oct 22, 2020

Uh oh!

jonahwilliams 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!

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!

Uh oh!

google-cla bot commented Oct 28, 2020

Uh oh!

karamage commented Oct 28, 2020

Uh oh!

google-cla bot commented Oct 28, 2020

Uh oh!

royarg02 commented Oct 28, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

royarg02 Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

karamage commented Oct 18, 2020 •

edited

Loading

royarg02 Oct 29, 2020 •

edited

Loading

christopherfujino Oct 30, 2020 •

edited

Loading

royarg02 Nov 23, 2020 •

edited

Loading

royarg02 Nov 23, 2020 •

edited

Loading

royarg02 commented Nov 23, 2020 •

edited

Loading