Skip to content

Use real flutter root for finding pub in command integration test#64021

Merged
jmagman merged 1 commit intoflutter:masterfrom
jmagman:pub-root
Aug 19, 2020
Merged

Use real flutter root for finding pub in command integration test#64021
jmagman merged 1 commit intoflutter:masterfrom
jmagman:pub-root

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Aug 18, 2020 •
edited
Loading

Description

This "hermetic" test uses real pub.

FlutterCommandRunner.initFlutterRoot(); was setting the cache root to ../...
analyze_continuously_test runs pub get relative to the temp directory, and ../../bin/cache/dart-sdk/bin/pub doesn't exist there.

Related Issues

Fixes #64008

Checklist

  • 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

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

@jmagman jmagman added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 18, 2020
@jmagman jmagman requested a review from jonahwilliams August 18, 2020 04:59
@jmagman jmagman self-assigned this Aug 18, 2020
jmagman
jmagman commented Aug 18, 2020
View reviewed changes
packages/flutter_tools/test/commands.shard/hermetic/analyze_continuously_test.dart
Logger logger;

setUp(() {
platform = const LocalPlatform();
Copy link
Member Author

@jmagman jmagman Aug 18, 2020

Choose a reason for hiding this comment

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

This was set twice

Copy link
Contributor

@christopherfujino christopherfujino Aug 18, 2020

Choose a reason for hiding this comment

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

copy-paste strikes again

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Aug 18, 2020

Was this just passing because of the CWD on cirrus? additionally, should we move this to the integration shard to make it clear it is absolutely not a unit test?

christopherfujino
christopherfujino approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM.

For a later PR, should we move this to permeable?

@jmagman
Copy link
Member Author

jmagman commented Aug 18, 2020 •
edited
Loading

Was this just passing because of the CWD on cirrus? additionally, should we move this to the integration shard to make it clear it is absolutely not a unit test?

When I run SHARD=tool_tests SUBSHARD=commands dart test.dart locally it passes, the cache flutter root state seems to be leaking between tests.

I tried to track down the leak but I couldn't spot it, I should probably spend more time on that but I got distracted.

@christopherfujino
Copy link
Contributor

christopherfujino commented Aug 18, 2020

Was this just passing because of the CWD on cirrus? additionally, should we move this to the integration shard to make it clear it is absolutely not a unit test?

When I run SHARD=tool_tests SUBSHARD=commands dart test.dart locally it passes, the cache flutter root state seems to be leaking between tests.

I tried to track down the leak but I couldn't spot it, I should probably spend more time on that but I got distracted.

Does test.dart run in a different order than pub run test? Or is test.dart running from a different working directory?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Aug 18, 2020

test.dart runs with FLUTTER_ROOT environment variable set

jonahwilliams
jonahwilliams approved these changes Aug 18, 2020
View reviewed changes
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

@jmagman jmagman merged commit 6ca9cd7 into flutter:master Aug 19, 2020
@jmagman jmagman deleted the pub-root branch August 19, 2020 00:09
smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020
@jmagman @smadey
Use real flutter root for finding pub in command integration test (fl…
fa3fdc8 
…utter#64021)
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@jmagman @mingwandroid
Use real flutter root for finding pub in command integration test (fl…
278bed5 
…utter#64021)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@christopherfujino christopherfujino christopherfujino approved these changes

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[flutter_tools] commands.shard analyze_continuously_test.dart failing locally

4 participants

@jmagman @jonahwilliams @christopherfujino @googlebot