[tools] Add Xcode version to non-verbose Flutter doctor #87022

TahaTesser · 2021-07-26T00:17:08Z

Before

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel master, 2.4.0-5.0.pre.180, on macOS 11.4 20F71 darwin-x64, locale en-US)
[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
[✓] Xcode - develop for iOS and macOS
[✓] Chrome - develop for the web

After

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel xcode_non_verbose_version, 2.4.0-5.0.pre.183, on macOS 11.4 20F71 darwin-x64, locale
    en-US)
[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
[✓] Xcode - develop for iOS and macOS (version 12.5.0)
[✓] Chrome - develop for the web

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 feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jmagman

Instead of adding versionInfo to ValidationResult which won't be used by any of the existing validators that report a version (and will therefore be unexpectedly null) and it looks like is only used in the unit test?, instead fix the regression introduced in #73808 and just set xcodeVersionInfo with its , parsing logic.

      if (versionText != null) {
        xcodeVersionInfo = versionText;
        if (xcodeVersionInfo.contains(',')) {
          xcodeVersionInfo = xcodeVersionInfo.substring(0, xcodeVersionInfo.indexOf(','));
        }

jmagman · 2021-07-27T17:34:48Z

packages/flutter_tools/lib/src/doctor_validator.dart


 import 'package:meta/meta.dart';

+import './base/version.dart';


Suggested change

import './base/version.dart';

import 'base/version.dart';

Now this can be removed.

Suggested change

import './base/version.dart';

jmagman · 2021-07-27T17:46:56Z

packages/flutter_tools/lib/src/doctor_validator.dart

  // A short message about the status.
  final String? statusInfo;
  final List<ValidationMessage> messages;
+  final Version? versionInfo;


This is only used in the unit test?

Revert.

Suggested change

final Version? versionInfo;

jmagman · 2021-07-27T18:19:37Z

packages/flutter_tools/test/general.shard/macos/xcode_validator_test.dart

+      );
+      final XcodeValidator validator = XcodeValidator(xcode: xcode, userMessages: UserMessages());
+      final ValidationResult result = await validator.validate();
+      expect('${validator.title} (version ${result.versionInfo})', 'Xcode - develop for iOS and macOS (version 12.0.1)');


You're doing the string concatenation in the test and then checking it, which only verifies your test, not the real code. Instead, a test like this should check that the actual code produces the expected string. In this case, result.statusInfo. Also, the xcode validator doesn't have a concept of doctor verbosity, you're just checking that the result status info is correct.

I suspect you probably got stuck because XcodeProjectInterpreter.test doesn't set up _versionText since this is the first test that needs it. The XcodeProjectInterpreter._ constructor can set _versionText.

flutter/packages/flutter_tools/lib/src/ios/xcodeproj.dart

Lines 60 to 61 in 59f201f

_version = version,

_usage = usage;

becomes

_version = version, _versionText = version?.toString(), _usage = usage;

A better way to test this change would be to check result.statusInfo and result.messages on every existing test.

testWithoutContext('Emits missing status when Xcode is not installed', () async { ... expect(result.type, ValidationType.missing); expect(result.statusInfo, isNull); expect(result.messages.last.type, ValidationMessageType.error); expect(result.messages.last.message, contains('Xcode not installed')); }); ... testWithoutContext('Succeeds when all checks pass', () async { ... expect(result.type, ValidationType.installed); expect(result.statusInfo, '12.0.1'); expect(result.messages.length, 2); final ValidationMessage firstMessage = result.messages.first; expect(firstMessage.type, ValidationMessageType.information); expect(firstMessage.message, 'Xcode at /Library/Developer/CommandLineTools'); final ValidationMessage lastMessage = result.messages.last; expect(lastMessage.type, ValidationMessageType.information); expect(lastMessage.message, '1000.0.0'); });

christopherfujino · 2021-08-12T20:56:40Z

@TahaTesser are you still working on this?

…rbose_version

TahaTesser · 2021-08-15T18:29:50Z

@jmagman
That was interesting, I made the changes in the code and tests, please take a look, thank you

jmagman · 2021-08-16T22:52:50Z

packages/flutter_tools/lib/src/base/user_messages.dart

      '(or visit ${_androidSdkInstallUrl(platform)} for detailed instructions).';

  // Messages used in XcodeValidator
+  String xcodeVersion(String version) => 'version $version';


This is unused.

Suggested change

String xcodeVersion(String version) => 'version $version';

jmagman · 2021-08-16T22:54:02Z

packages/flutter_tools/lib/src/doctor_validator.dart


 import 'package:meta/meta.dart';

+import './base/version.dart';


Now this can be removed.

Suggested change

import './base/version.dart';

jmagman · 2021-08-16T22:54:24Z

packages/flutter_tools/lib/src/doctor_validator.dart

  /// [ValidationResult.type] should only equal [ValidationResult.installed]
  /// if no [messages] are hints or errors.
-  const ValidationResult(this.type, this.messages, { this.statusInfo });
+  const ValidationResult(this.type, this.messages, { this.statusInfo, this.versionInfo });


Revert this.

Suggested change

const ValidationResult(this.type, this.messages, { this.statusInfo, this.versionInfo });

const ValidationResult(this.type, this.messages, { this.statusInfo });

jmagman · 2021-08-16T22:54:32Z

packages/flutter_tools/lib/src/doctor_validator.dart

  // A short message about the status.
  final String? statusInfo;
  final List<ValidationMessage> messages;
+  final Version? versionInfo;


Revert.

Suggested change

final Version? versionInfo;

jmagman · 2021-08-16T22:54:45Z

packages/flutter_tools/lib/src/macos/xcode_validator.dart

    }

-    return ValidationResult(xcodeStatus, messages, statusInfo: xcodeVersionInfo);
+    return ValidationResult(xcodeStatus, messages, statusInfo: xcodeVersionInfo, versionInfo: _xcode.currentVersion);


Revert.

Suggested change

return ValidationResult(xcodeStatus, messages, statusInfo: xcodeVersionInfo, versionInfo: _xcode.currentVersion);

return ValidationResult(xcodeStatus, messages, statusInfo: xcodeVersionInfo);

jmagman · 2021-08-16T22:59:45Z

packages/flutter_tools/test/general.shard/macos/xcode_validator_test.dart

      expect(result.messages.last.message, contains('Flutter requires a minimum Xcode version of 12.0.1'));
    });

+    testWithoutContext('Check for Xcode version in non-verbose flutter doctor', () async {


Also, the xcode validator doesn't have a concept of doctor verbosity, you're just checking that the result status info is correct.

See my comment in #87022 (comment), this test is the same case as Succeeds when all checks pass so it doesn't add any test coverage. So you can remove it, you're covered by the new statusInfo expectations.

If you want you can add the ValidationMessage test expectations I already wrote in that comment, but that's optional.

…rbose_version

TahaTesser · 2021-08-17T19:55:23Z

@jmagman
I should have reverted these changes earlier for versionInfo, my bad, now it's done, still getting used to get to this process

expect(result.messages.length, 2);

messages length is only 1, so I have updated the test for the first message only

Thank you so much!

jmagman

LGTM, thanks @TahaTesser!

TahaTesser · 2021-08-17T21:18:05Z

packages/flutter_tools/test/general.shard/macos/xcode_validator_test.dart

      );
      final XcodeValidator validator = XcodeValidator(xcode: xcode, userMessages: UserMessages());
      final ValidationResult result = await validator.validate();
      expect(result.type, ValidationType.installed);


@jmagman
This was accidentally removed, restored it cb7aa1b

TahaTesser added 2 commits July 26, 2021 01:52

Add Xcode version to non-verbose doctor

8a69b60

test and refactor

efba0bd

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 26, 2021

TahaTesser requested a review from jmagman July 26, 2021 00:17

google-cla bot added the cla: yes label Jul 26, 2021

jmagman requested changes Jul 27, 2021

View reviewed changes

christopherfujino added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Aug 12, 2021

TahaTesser added 2 commits August 15, 2021 20:43

Merge branch 'master' of github.com:flutter/flutter into xcode_non_ve…

fbb03ef

…rbose_version

updated code and test

414282e

TahaTesser requested a review from jmagman August 15, 2021 18:29

TahaTesser removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Aug 15, 2021

jmagman requested changes Aug 16, 2021

View reviewed changes

TahaTesser added 4 commits August 17, 2021 21:14

Merge branch 'master' of github.com:flutter/flutter into xcode_non_ve…

997c038

…rbose_version

Merge branch 'master' of github.com:flutter/flutter into xcode_non_ve…

4e6c439

…rbose_version

revert previous changes and update test

de6498b

update test

a1b760e

TahaTesser requested a review from jmagman August 17, 2021 19:55

jmagman approved these changes Aug 17, 2021

View reviewed changes

jmagman added the waiting for tree to go green label Aug 17, 2021

restore ValidationType.installed

cb7aa1b

TahaTesser commented Aug 17, 2021

View reviewed changes

fluttergithubbot merged commit 42a6b79 into flutter:master Aug 17, 2021

TahaTesser deleted the xcode_non_verbose_version branch August 17, 2021 22:59

blasten pushed a commit to blasten/flutter that referenced this pull request Aug 19, 2021

[tools] Add Xcode version to non-verbose Flutter doctor (flutter#87022)

bd45088


		import 'package:meta/meta.dart';

		import './base/version.dart';

	const ValidationResult(this.type, this.messages, { this.statusInfo, this.versionInfo });
	const ValidationResult(this.type, this.messages, { this.statusInfo });

	return ValidationResult(xcodeStatus, messages, statusInfo: xcodeVersionInfo, versionInfo: _xcode.currentVersion);
	return ValidationResult(xcodeStatus, messages, statusInfo: xcodeVersionInfo);

[tools] Add Xcode version to non-verbose Flutter doctor #87022

[tools] Add Xcode version to non-verbose Flutter doctor #87022

Uh oh!

Conversation

TahaTesser commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Pre-launch Checklist

Uh oh!

jmagman left a comment • 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!

jmagman Jul 27, 2021 • 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!

christopherfujino commented Aug 12, 2021

Uh oh!

TahaTesser commented Aug 15, 2021

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!

jmagman Aug 16, 2021 • 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!

TahaTesser commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TahaTesser commented Jul 26, 2021 •

edited

Loading

jmagman left a comment •

edited

Loading

jmagman Jul 27, 2021 •

edited

Loading

jmagman Aug 16, 2021 •

edited

Loading

TahaTesser commented Aug 17, 2021 •

edited

Loading