Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update flutter_devtools.test #314

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Update flutter_devtools.test #314

merged 2 commits into from
Nov 22, 2023

Conversation

zanderso
Copy link
Member

To pick up flutter/devtools#6818, to unblock Dart -> Flutter rolls.

To pick up flutter/devtools#6818, to unblock Dart -> Flutter rolls.
eyebrowsoffire
eyebrowsoffire previously approved these changes Nov 22, 2023
Copy link

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

LGTM!

@zanderso
Copy link
Member Author

Hi @DanTup, I'm trying to roll flutter/devtools into flutter/tests, but it looks like there's an issue with the change here flutter/devtools#6776, which is causing tools/bin/devtools_tool to fail to find the Dart SDK when invoked from tools/ci/setup.sh. It looks like it's only failing on Linux, so maybe something related to the way that (environment) variables are set from the shell scripts?

@DanTup
Copy link
Contributor

DanTup commented Nov 22, 2023

I'm not sure why things wouldn't be consistent across platforms, but do you know where the Flutter SDK is being set up in this case? It looks like flutter_customer_tests/setup.sh assumes Flutter already exists but I'm not sure where it's set up.

In the PR you linked, we made devtools_tool always expect to use a Flutter SDK from the tool/flutter-sdk folder (whereas before, we could pick it up from PATH too). The reason for this was that DevTools is pinning specific versions of Flutter and we didn't want to accidentally get a different version from a users PATH.

However, it's not clear to me if the tests here want to always run with the latest one (in which case we may need a way to override this, or we need it tbe cloned or symlinked into tool/flutter-sdk).

(cc @kenzieschmoll in case she has any opinions/preferences on how this is solved)

@zanderso
Copy link
Member Author

The purpose of the customer tests is to test against tip-of-tree Flutter, to ensure that changes to Flutter don't unintentionally break those tests. So in that scenario, running against the Flutter SDK from the environment rather that one pinned to an earlier version would be working as intended.

@zanderso
Copy link
Member Author

@DanTup, offline @jacob314 recommended we revert at flutter/devtools#6820 to unblock the rolls.

@DanTup
Copy link
Contributor

DanTup commented Nov 22, 2023

Makes sense to me - I'll tweak it to support this (perhaps by adding an env variable to allow forcing use of Flutter from PATH). Thanks!

Copy link

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

LGTM!

@zanderso zanderso merged commit 1438b9a into main Nov 22, 2023
12 checks passed
@zanderso zanderso deleted the zanderso-patch-1 branch November 22, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants