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

The flutter test command should require a flutter_test or test_api dependency (and check for it). #91107

Open
jakemac53 opened this issue Oct 1, 2021 · 15 comments
Assignees
Labels
a: tests "flutter test", flutter_test, or one of our tests c: proposal A detailed proposal for a change to Flutter P2 Important issues not at the top of the work list team-tool Owned by Flutter Tool team tool Affects the "flutter" command-line tool. See also t: labels. triaged-tool Triaged by Flutter Tool team

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented Oct 1, 2021

Use case

The flutter test command itself is built with a certain version of the test package (whatever is pinned in the repo at the time the SDK was published). If a user is only using the regular test package, then there is nothing to ensure that the version is compatible with that of the flutter test command.

See #91059 as an example failure.

Proposal

The flutter test command should check for a dependency on flutter_test, and fail (or at least warn) if one does not exist. This package pins the test_api package to a compatible version, solving the issue.

@jakemac53 jakemac53 added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. labels Oct 1, 2021
@Hixie
Copy link
Contributor

Hixie commented Oct 1, 2021

Would it make sense to test for the right version of test_api, rather than flutter_test? (In the message we should suggest depending on flutter_test, certainly, but I feel like if someone is doing the dance to point to the right test_api version that should work too; they don't have to use flutter_test with flutter test.)

@jakemac53
Copy link
Contributor Author

jakemac53 commented Oct 1, 2021

I would probably just depend on flutter_test, as it will take into account all the possible deps and make sure they are compatible. Being an sdk dependency it is already downloaded and essentially free (assuming it's a dev dependency).

In practice it's likely that just enforcing test_api is compatible would fix most issues though.

@Hixie
Copy link
Contributor

Hixie commented Oct 1, 2021

I guess asking for it to be in the pubspec.yaml as a dev dependency is probably ok. It doesn't require actually importing it.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Mar 23, 2023

Note dart-lang/test#1977, this is another good example of why we should be requiring this dependency even if people don't use the package as an import.

@natebosch
Copy link
Contributor

This is also what webdev does - it checks for appropriate versions of packages like build_web_compilers.

If any tool is going to operate with packages from the pub solve, but not be installed with that same pub solve, it should validate any assumptions it makes about how it can operate with those packages.

From the user standpoint a dev_dependency on the SDK vendored version of flutter_test is a far superior UX to a pinned dependency on test_api. With the former they can be compatible across Flutter SDK releases.

@polina-c
Copy link
Contributor

DevTools just faced the issue

@guyluz11
Copy link

Not sure if it was mentiond already but wanted to point out that in the documentation of flutter it is written that

Add the test or flutter_test dependency.

https://docs.flutter.dev/cookbook/testing/unit/introduction

@jakemac53
Copy link
Contributor Author

That documentation should also be updated as well to just always tell you to add the flutter_test dependency I suppose?

It feels a bit weird, but it does make sense, based on how the flutter test command ships. If you run dart test without a test dependency it will make you add that dependency, FWIW. So it also makes sense for flutter test to require a flutter_test dependency, even if it technically can work without it, it isn't safe to run it without it.

natebosch added a commit to natebosch/super_editor that referenced this issue Mar 24, 2023
This package does not have a dependency on `flutter_test` and so there
is no guarantee that the version of `test_api` from the pub solve is
compatible with the `flutter test` command.

See flutter/flutter#91107
@Hixie Hixie changed the title The flutter test command should require a flutter_test dependency. The flutter test command should require a flutter_test or test_api dependency (and check for it). Mar 24, 2023
@Hixie Hixie added the P2 Important issues not at the top of the work list label Mar 24, 2023
@devoncarew
Copy link
Member

@christopherfujino - is this something we could address in the near term? We see issues related to this when we make changes to package:test_api (and related). We have some near term refactoring we'd like to do to help separate out package:test and package:matcher (and eventually allow us to support the newer package:checks).

I believe the desired resolution here is still the one from the first comment? (The flutter test command should check for a dependency on flutter_test, and fail (or at least warn) if one does not exist.)

@christopherfujino
Copy link
Member

@christopherfujino - is this something we could address in the near term? We see issues related to this when we make changes to package:test_api (and related). We have some near term refactoring we'd like to do to help separate out package:test and package:matcher (and eventually allow us to support the newer package:checks).

I believe the desired resolution here is still the one from the first comment? (The flutter test command should check for a dependency on flutter_test, and fail (or at least warn) if one does not exist.)

SGTM

@christopherfujino christopherfujino self-assigned this Jun 6, 2023
@flutter-triage-bot flutter-triage-bot bot added team-tool Owned by Flutter Tool team triaged-tool Triaged by Flutter Tool team labels Jul 8, 2023
@flutter-triage-bot flutter-triage-bot bot added the Bot is counting down the days until it unassigns the issue label Nov 25, 2023
@flutter-triage-bot
Copy link

This issue is assigned to @christopherfujino but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

@jakemac53
Copy link
Contributor Author

jakemac53 commented Dec 1, 2023

@christopherfujino this is about to come up again when we change the wire format for the Runtime class here dart-lang/test#2144. This type of change should not be breaking but it ends up breaking because there is no forced constraint on package:test_api when using flutter test, so the wire format between the tool and the package don't line up.

I think we can probably make it backwards compatible, but it would be great if we didn't have to, otherwise we are stuck with this tech debt until we decide to knowingly break users on some older version of flutter.

@christopherfujino
Copy link
Member

@christopherfujino this is about to come up again when we change the wire format for the Runtime class here dart-lang/test#2144. This type of change should not be breaking but it ends up breaking because there is no forced constraint on package:test_api when using flutter test, so the wire format between the tool and the package don't line up.

I think we can probably make it backwards compatible, but it would be great if we didn't have to, otherwise we are stuck with this tech debt until we decide to knowingly break users on some older version of flutter.

@jakemac53 thanks for the heads up! Assigning @eliasyishak to fix.

@christopherfujino
Copy link
Member

@eliasyishak is this still on your radar?

@eliasyishak
Copy link
Contributor

Still tracking on my todos but haven't got around to it yet as I figure out how to handle the breaking change that comes with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests c: proposal A detailed proposal for a change to Flutter P2 Important issues not at the top of the work list team-tool Owned by Flutter Tool team tool Affects the "flutter" command-line tool. See also t: labels. triaged-tool Triaged by Flutter Tool team
Projects
None yet
Development

No branches or pull requests

10 participants