ahh yeah, we already support that :)

Although, isn't gitTagVersion here equivalent to HEAD?

Yeah, I think this is incorrect. .frameworkVersionFor() is a confusingly named method. We would need to compute a new version based on upstreamRevision. I'm pretty sure we need an additional invocation to git to either read the tag on the commit (for release channels) or git describe if we're on master.

So something like git describe --tags --abbrev=0 would suffice? Seems working to me running in my cloned repo.

So something like git describe --tags --abbrev=0 would suffice? Seems working to me running in my cloned repo.

Unfortunately, if the commit in question is not a tagged commit, that will report the nearest tag. However, if a commit is 7 commits past version 1.2.3, it is not accurate to report that it as 1.2.3. What we need is to support constructing a GitTagVersion for a revision other than HEAD. Then I would update the function Future<String> fetchRemoteRevision() to instead be Future<FlutterVersion> fetchLatestVersion().

The problem is that GitTagVersion.determine() has this line:

    final List<String> tags = _runGit(
      'git tag --points-at HEAD', processUtils, workingDirectory).trim().split('\n');

which hard-codes HEAD.

And then also the fallback at the end implicitly checks HEAD:

    // If we're not currently on a tag, use git describe to find the most
    // recent tag and number of commits past.
    return parse(
      _runGit(
        'git describe --match *.*.* --first-parent --long --tags',
        processUtils,
        workingDirectory,
      )
    );

What we need is to support constructing a GitTagVersion for a revision other than HEAD.

Changing that would break most of the tests in version_test.dart.

Then I would update the function Future<String> fetchRemoteRevision() to instead be Future<FlutterVersion> fetchLatestVersion().

Changing this would break tests at upgrade_test.dart as well.

I think this PR might be a breaking change. I got to know from the chat that this PR doesn't necessarily has to go through the breaking changes policy, but we still need to handle the failing tests due to this change.

You can delete this TODO comment

OK

Yeah, I think this is incorrect. .frameworkVersionFor() is a confusingly named method. We would need to compute a new version based on upstreamRevision. I'm pretty sure we need an additional invocation to git to either read the tag on the commit (for release channels) or git describe if we're on master.

This comment is just a hint for later--first we need to update commands/upgrade.dart with my comments there.

This is not your fault, you were copying an already incorrect test (that I probably wrote), but we should not compute the expectation text, but rather hard-code it. Otherwise, if the functions generating the test is wrong, the test will still pass. For example, when I add print logs to see what we're actually asserting on:

00:04 +3: UpgradeCommandRunner Correctly provides upgrade version on verify only
latest: 0.0.0-unknown
current: null

This seems of minimal value.

When we get the tool doing the right thing I can help you improve these tests.

Everything here looks good, I think we just need to address this comment. I think the expectation text here should be raw strings, and not calling the methods. That way if someone inadvertently changes the behavior of these methods, the test will fail, signaling the change.

MockFlutterVersion is not using any defined GitTagVersion, so the tests are showing the "version" as null.

The latest version: null (revision def456)
Your current version: null (revision abc123)

Although the file has this GitTagVersion instance, it is only used as an argument to fakeCommandRunner.runCommand:

The revision can be shown if we use when(MockFlutterVersion().frameworkRevisionShort).thenReturn(revision) either in the test or directly in MockFlutterVersion's constructor:

class MockFlutterVersion extends Mock implements FlutterVersion {
  MockFlutterVersion({bool isStable = false, String revision}) : _isStable = isStable {
    //This is only for other tests that depend on FakeUpgradeCommandRunner.remoteRevision
    when(frameworkRevision).thenReturn(revision);
    when(frameworkRevisionShort).thenReturn(revision);
  }

You can use the ??= operator here, like: _frameworkRevision ??= _runGit(

Can I recommend you name this argument String gitRef, so as to be more general, if in the future we want to pass something other than a hash.

And if you set the default value to be 'HEAD', then you don't have to do all the fallbacks in the rest of this function.

Please add a space between the version and the parentheses here and on the next line. This currently looks like:

The latest version: 1.24.0-8.0.pre.5(revision 1f15046b93)
Your current version: 1.24.0-8.0.pre.6(revision bf0fc66cbf)

We should also ensure that in the unit tests we're asserting that this is formatted correctly.

Can you change this from null to an actual version tag? Your update to FakeUpgradeCommandRunner.fetchLatestVersion is constructing a new MockFlutterVersion on the fly, which is returning null when .frameworkVersion is invoked. Probably the easiest way to fix this is to set a new property on FakeUpgradeCommandRunner called FlutterVersion frameworkVersion; that can be optionally set in the constructor.

Probably the easiest way to fix this is to set a new property on FakeUpgradeCommandRunner called FlutterVersion frameworkVersion; that can be optionally set in the constructor.

Could you elaborate how this would solve the problem? I think that MockFlutterVersion.frameworkVersion is returning null as it is not assigned, and can be fixed by the when-thenReturn construct just like frameworkRevision:

class FakeUpgradeCommandRunner extends UpgradeCommandRunner {
  bool willHaveUncommittedChanges = false;

  bool alreadyUpToDate = false;

  String remoteRevision = '';

  String remoteVersion = '';

  @override
  Future<FlutterVersion> fetchLatestVersion() async => MockFlutterVersion(revision: remoteRevision, version: remoteVersion);

class MockFlutterVersion extends Mock implements FlutterVersion {
  MockFlutterVersion({String revision, String version}) {
    when(frameworkRevision).thenReturn(revision);
    when(frameworkRevisionShort).thenReturn(revision);
    when(frameworkVersion).thenReturn(version);
  }
}

We would have to then explicitly provide a version number for expectations to be meaningful.

In the tool code, we explicitly set mock return values in the test code, so that the mocked behavior is obvious when reading the test. It will make updating the test easier if, for example, the behavior of fetchLatestVersion() were to change in a future test.

Something like this:

class FakeUpgradeCommandRunner extends UpgradeCommandRunner {
  bool willHaveUncommittedChanges = false;
  bool alreadyUpToDate = false;

  String remoteRevision;
  FlutterVersion remoteVersion;

  @override
  Future<FlutterVersion> fetchLatestVersion() async => remoteVersion;
}

In the tool code, we explicitly set mock return values in the test code

This snippet alongwith snippets in my previous comment show how that can be done:

const String revision = 'abc123';
const String upstreamRevision = 'def456';
const String version = '1.2.3';
const String upstreamVersion = '4.5.6';
when(flutterVersion.frameworkRevision).thenReturn(revision);
when(flutterVersion.frameworkRevisionShort).thenReturn(revision);
fakeCommandRunner.remoteVersion = upstreamVersion;
fakeCommandRunner.remoteRevision = upstreamRevision;

Also I have some issues with the snippet you've provided:

String remoteRevision;
FlutterVersion remoteVersion;

@override
Future<FlutterVersion> fetchLatestVersion() async => remoteVersion;

• Making this change would require all tests to explicitly define FakeUpgradeCommandRunner.remoteVersion.
• What do we assign to FakeUpgradeCommandRunner.remoteVersion in the tests, MockFlutterVersion or FlutterVersion?
• If we assign MockFlutterVersion, we can just simply, for example, do:

MockFlutterVersion latestVersion = MockFlutterVersion();
when(latestVersion.frameworkRevision).thenReturn(upstreamRevision);
when(latestVersion.frameworkRevisionShort).thenReturn(upstreamRevision);
when(latestVersion.frameworkVersion).thenReturn(upstreamVersion);
fakeCommandRunner.remoteVersion = latestVersion;

or

fakeCommandRunner.remoteVersion = MockFlutterVersion(revision: remoteRevision, version: remoteVersion);

Having FakeUpgradeCommandRunner.remoteRevision will be redundant in either case.

• If we assign FlutterVersion, processManager would have additional instructions, and the expectation will fail. If we assign MockFlutterVersion, it will not have a "version" tagged by default unless we explicitly provide one using either of the two above.

I'm sorry that I cannot follow your approach, but could you provide me an example on how FakeUpgradeCommandRunner.remoteRevision would be used in the tests?

In the tool code, we explicitly set mock return values in the test code

This snippet alongwith snippets in my previous comment show how that can be done:

const String revision = 'abc123';
const String upstreamRevision = 'def456';
const String version = '1.2.3';
const String upstreamVersion = '4.5.6';
when(flutterVersion.frameworkRevision).thenReturn(revision);
when(flutterVersion.frameworkRevisionShort).thenReturn(revision);
fakeCommandRunner.remoteVersion = upstreamVersion;
fakeCommandRunner.remoteRevision = upstreamRevision;

Also I have some issues with the snippet you've provided:

String remoteRevision;
FlutterVersion remoteVersion;

@override
Future<FlutterVersion> fetchLatestVersion() async => remoteVersion;

• Making this change would require all tests to explicitly define FakeUpgradeCommandRunner.remoteVersion.

If runner.remoteVersion isn't read in the test, it does not need to be set, right? We can leave it null. If it IS read during the test, we should force the test author to explicitly set the property with a mock object, to ensure that we don't get false positives for new tests. Where possibly, we try to write our tests so that behavior that isn't explicitly set will fail the test, so that the author gets signal that they need to mock a certain behavior.

• What do we assign to FakeUpgradeCommandRunner.remoteVersion in the tests, MockFlutterVersion or FlutterVersion?
• If we assign MockFlutterVersion, we can just simply, for example, do:

MockFlutterVersion latestVersion = MockFlutterVersion();
when(latestVersion.frameworkRevision).thenReturn(upstreamRevision);
when(latestVersion.frameworkRevisionShort).thenReturn(upstreamRevision);
when(latestVersion.frameworkVersion).thenReturn(upstreamVersion);
fakeCommandRunner.remoteVersion = latestVersion;

or

fakeCommandRunner.remoteVersion = MockFlutterVersion(revision: remoteRevision, version: remoteVersion);

Yes, the former is more verbose. We favor longer tests with explicit mocked behavior. The former example is easier for a contributor to read and know exactly what is mocked test behavior. Consider, for example, a future test wants to ensure that a frameworkRevisionShort is a subset of frameworkRevision. It will be clear in the former implementation that both values are manually mocked, and how to update the test.

• If we assign FlutterVersion, processManager would have additional instructions, and the expectation will fail. If we assign MockFlutterVersion, it will not have a "version" tagged by default unless we explicitly provide one using either of the two above.

This is a good point. You can go either way, but passing a MockFlutterVersion and mocking the return value of frameworkVersion would be easier. This would still ensure that we're reading the version label and interpolating it into stdout. If you want to pass a real FlutterVersion, you would need to update FakeProcessManager, with the method processManager.addCommands().

I don't think this is necessary? The tests pass without it. And I'm not familiar the version property of Platform.

Yes, it isn't. Probably a leftover I overlooked. I removed it.