Skip to content

Conversation

franciscojma86
Copy link
Contributor

Description

Fixes a crash introduced on #40011 due to an incorrect type in the vswhere search

Related Issues

Fixes #40238

Tests

Relevant tests in visual_studio_test.dart were updated

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 11, 2019
@franciscojma86 franciscojma86 added c: crash Stack traces logged to the console platform-windows Building on or for Windows specifically and removed cla: yes labels Sep 11, 2019
@franciscojma86 franciscojma86 added the t: flutter doctor Problem related to the "flutter doctor" tool label Sep 11, 2019
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

'productMilestoneIsPreRelease': true,
}
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a variant of this (_oldResponse?) that doesn't contain the reboot/complete/launchable entries, and at least one test using them; installationHasIssues still has asserts that the keys are present; we should have a test coverage that would catch that. (And also, please fix that 😀)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the change above

assert(installationDetails[_isLaunchableKey] != null);
if (installationDetails[_isCompleteKey] == null ||
installationDetails[_isRebootRequiredKey] == null ||
installationDetails[_isLaunchableKey] == null) {
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather we not treat these as a monolithic block, where we don't check them at all if any one is missing. Instead this can be:

if (installationDetails[_isCompleteKey] != null && !installationDetails[_isCompleteKey]) {
  return true;
}
[similarly for the others]
return false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@8eee93f). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #40263   +/-   ##
=========================================
  Coverage          ?   58.42%           
=========================================
  Files             ?      192           
  Lines             ?    18539           
  Branches          ?        0           
=========================================
  Hits              ?    10831           
  Misses            ?     7708           
  Partials          ?        0
Flag Coverage Δ
#flutter_tool 58.42% <87.5%> (?)
Impacted Files Coverage Δ
...s/flutter_tools/lib/src/windows/visual_studio.dart 79.41% <87.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8eee93f...4d47813. Read the comment docs.

};

// A version of a response that doesn't include certain installation status
// information that might be missing in older Windows versions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Windows/Visual Studio/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here and in other places

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with changes noted above, with the understanding that the potential crashes will be fixed in the other PR ASAP.

@stuartmorgan-g stuartmorgan-g merged commit f5733f7 into flutter:master Sep 12, 2019
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
Fixes a crash introduced on flutter#40011 due to an incorrect type in the vswhere search

Fixes flutter#40238
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: crash Stack traces logged to the console platform-windows Building on or for Windows specifically t: flutter doctor Problem related to the "flutter doctor" tool tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows 7 desktop, flutter doctor crash

5 participants