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.

Sign up for GitHub

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

Jump to bottom

video_player depends on flutter_test #83562

Closed
tjarvstrand opened this issue May 28, 2021 · 14 comments · Fixed by flutter/plugins#4627
Closed

video_player depends on flutter_test #83562

tjarvstrand opened this issue May 28, 2021 · 14 comments · Fixed by flutter/plugins#4627
Assignees
@stuartmorgan
Labels
p: video_player The Video Player plugin P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. r: fixed Issue is closed as already fixed in a newer version

Comments

@tjarvstrand
Copy link

I believe this should be a dev dependency?

I don't use flutter_test myself but now I'm unable to upgrade one of my dependencies because I do use video_player and the other dependency is (regrettably) incompatible with flutter_test.
@stuartmorgan
Copy link
Contributor

It was moved in flutter/plugins#3281

@Hixie @gaaclarke does pigeon require test libraries to be non-dev dependencies for some reason?

@TahaTesser TahaTesser added in triage Presently being triaged by the triage team p: first party p: video_player The Video Player plugin and removed in triage Presently being triaged by the triage team labels May 31, 2021
@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2021

Yes, some of the APIs generated by pigeon are test APIs that themselves depend on flutter_test. We could make pigeon generate two libraries (i.e. two pubspec.yaml files), but I'm not familiar enough with how pigeon's generated libraries are used to know if that makes sense (e.g. would you need to upload two packages to pub? How many of the libraries in a federated plugin would need to be duplicated?).

@stuartmorgan
Copy link
Contributor

Doesn't pigeon just generate sources, not packages/libraries?

@gaaclarke
Copy link
Member

Yea, Pigeon just generates source code, not libraries. You just specify the paths of where you want to generate the source code. I don't see a reason why a plugin should have flutter_test as a dependency and Pigeon itself can be a dev_dependency (or not a dependency at all if you have it globally installed).

If you want to use Pigeon to generate test code you'd just have it generate in the test/ directory just like where you'd put your handwritten test code.

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2021

https://github.com/flutter/plugins/blob/master/packages/video_player/video_player_platform_interface/lib/test.dart is an example of what I mean.

@stuartmorgan stuartmorgan added the P2 Important issues not at the top of the work list label Jun 3, 2021
@stuartmorgan stuartmorgan self-assigned this Jun 3, 2021
@gaaclarke
Copy link
Member

Shouldn't that just be in the test/ directory though, not the lib/ directory? It's all a fancy wrapper around setMockHandler which would only be relevant for tests.

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2021

Isn't it part of the video_player_platform_interface public API?

@gaaclarke
Copy link
Member

Ahh yea, that's interesting. It's part of the public API for the platform interface, but the only clients of it are tests in packages that depend on the platform interface. You are probably better off making a new library (which is outside of the purview of pigeon).

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2021

So we'd have video_player_platform_interface, video_player_platform_interface_test, video_player, and video_player_web?
Seems reasonable to me, though it's probably worth looking at in the context of @stuartmorgan's work on reexamining our experience with the federated model. I can totally imagine that it would make landing pigeon changes much harder (first land pigeon, then publish it, then land the change to video_player_platform_interface, then publish it, then land the change to video_player_platform_interface_test, then publish it, then land the change to video_player, then publish it...).

@stuartmorgan
Copy link
Contributor

If only test code should ever import that file, couldn't we just make flutter_test a dev dependency in the current structure?

@Hixie
Copy link
Contributor

Hixie commented Jun 4, 2021

If it's used by other packages, then it's not a dev dependency, it's a real dependency. dev_dependency doesn't mean "only used by tests", it means "not used by people who depend on this".

@stuartmorgan
Copy link
Contributor

the only clients of it are tests in packages that depend on the platform interface

Capturing a thought that I had while not at my computer to investigate further: why is this public at all? I.e., why are tests in other packages mocking the method channels at all, since they are an internal implementation detail of the method channel implementation?

Is this issue just a side effect of us not having correctly updated tests to use platform interface mocks instead of channel mocking in the other component packages when federating retroactively (which I know is a thing that happened in at least some of our packages)?

@spkersten spkersten mentioned this issue Aug 18, 2021
Closed
@nt4f04uNd
Copy link
Member

Is this issue just a side effect of us not having correctly updated tests to use platform interface mocks instead of channel mocking in the other component packages when federating retroactively (which I know is a thing that happened in at least some of our packages)?

This seems to be the case. Usually the app-facing package is tested by setting VideoPlayerPlatformInterface.instance = MockVideoPlayerPlatform();, but it's not how it's done in video_player's tests.

stuartmorgan added a commit to stuartmorgan/plugins that referenced this issue Dec 7, 2021
@stuartmorgan
[video_player] Eliminate platform channel from mock platform
e2ecc37 
When video_player was federated, the existing unit tests in the
app-facing package--which were based on mocking out the method
channel--were left as-is. Because of the use of Pigeon, and thus
Pigeon-generated mocks, this creates a dependency from the test code in
the app-facing package on test code that is published from the platform
interface package, which causes unwanted dependencies in the platform
interface.

This updates the tests in the app-facing package to follow best
practices for federated plugins, which is for the app-facing tests to
directly mock/fake the platform interface, not depend on the
method-channel-based implementation. This simplifies the tests, ensures
that they aren't also testing code from another package, and allows for
removing the test code from the public interface of the platform
interface package (in a seperate follow-up PR).

Part of flutter/flutter#83562
stuartmorgan added a commit to stuartmorgan/plugins that referenced this issue Dec 7, 2021
@stuartmorgan
[video_player] Eliminate platform channel from mock platform
a5dcb2d 
When video_player was federated, the existing unit tests in the
app-facing package--which were based on mocking out the method
channel--were left as-is. Because of the use of Pigeon, and thus
Pigeon-generated mocks, this creates a dependency from the test code in
the app-facing package on test code that is published from the platform
interface package, which causes unwanted dependencies in the platform
interface.

This updates the tests in the app-facing package to follow best
practices for federated plugins, which is for the app-facing tests to
directly mock/fake the platform interface, not depend on the
method-channel-based implementation. This simplifies the tests, ensures
that they aren't also testing code from another package, and allows for
removing the test code from the public interface of the platform
interface package (in a seperate follow-up PR).

In order to allow the tests to work, the controller's caching of the
platform instance is changed to allow it to notice changes.

Part of flutter/flutter#83562
This was referenced Dec 7, 2021
Merged
Merged
stuartmorgan added a commit to flutter/plugins that referenced this issue Dec 19, 2021
@stuartmorgan
[video_player] Remove test code from platform interface (#4589)
6531740 
This is a breaking change to video_player_platform_interface to remove Pigeon mocks from the public interface. These should never have been there, as tests in packages outside of the platform interface should be mocking the platform interface rather than the method channel, but the app-facing tests weren't rewritten when they should have been so they ended up published to support the legacy tests. This creates an unwanted non-dev dependency on `flutter_test`, in addition to promoting an anti-pattern.

While making the breaking change, this also adopts `PlatformInterface`, removing `isMock` in favor of the standard mixin pattern provided by the base class.

Part of flutter/flutter#83562
stuartmorgan added a commit to stuartmorgan/plugins that referenced this issue Dec 20, 2021
@stuartmorgan
[video_player] Add 5.0 interface support
2a45211 
Allows the other video_player packages to use either 4.x or 5.x of
`video_player_platform_interface`, since the only breaking change
doesn't affect them (as it just removes test code that they no longer
use). This allows clients of `video_player` to no longer have a
transitive dependency on test packages, but doesn't create any version
lock with any unendorsed implementations that might exist.

Fixes flutter/flutter#83562
@stuartmorgan stuartmorgan mentioned this issue Dec 20, 2021
Merged
11 tasks
stuartmorgan added a commit to flutter/plugins that referenced this issue Dec 20, 2021
@stuartmorgan
[video_player] Add 5.0 interface support (#4627)
c9c234a 
Allows the other video_player packages to use either 4.x or 5.x of
`video_player_platform_interface`, since the only breaking change
doesn't affect them (as it just removes test code that they no longer
use). This allows clients of `video_player` to no longer have a
transitive dependency on test packages, but doesn't create any version
lock with any unendorsed implementations that might exist.

Fixes flutter/flutter#83562
KyleFin pushed a commit to KyleFin/plugins that referenced this issue Dec 21, 2021
@stuartmorgan @KyleFin
[video_player] Remove test code from platform interface (flutter#4589)
8c343ba 
This is a breaking change to video_player_platform_interface to remove Pigeon mocks from the public interface. These should never have been there, as tests in packages outside of the platform interface should be mocking the platform interface rather than the method channel, but the app-facing tests weren't rewritten when they should have been so they ended up published to support the legacy tests. This creates an unwanted non-dev dependency on `flutter_test`, in addition to promoting an anti-pattern.

While making the breaking change, this also adopts `PlatformInterface`, removing `isMock` in favor of the standard mixin pattern provided by the base class.

Part of flutter/flutter#83562
KyleFin pushed a commit to KyleFin/plugins that referenced this issue Dec 21, 2021
@stuartmorgan @KyleFin
[video_player] Add 5.0 interface support (flutter#4627)
84891e3 
Allows the other video_player packages to use either 4.x or 5.x of
`video_player_platform_interface`, since the only breaking change
doesn't affect them (as it just removes test code that they no longer
use). This allows clients of `video_player` to no longer have a
transitive dependency on test packages, but doesn't create any version
lock with any unendorsed implementations that might exist.

Fixes flutter/flutter#83562
@github-actions
Copy link

github-actions bot commented Jan 3, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 3, 2022
ValentinVignal pushed a commit to ValentinVignal/flutter__packages that referenced this issue Feb 23, 2023
@stuartmorgan
[video_player] Add 5.0 interface support (flutter#4627)
87805a4 
Allows the other video_player packages to use either 4.x or 5.x of
`video_player_platform_interface`, since the only breaking change
doesn't affect them (as it just removes test code that they no longer
use). This allows clients of `video_player` to no longer have a
transitive dependency on test packages, but doesn't create any version
lock with any unendorsed implementations that might exist.

Fixes flutter/flutter#83562
zhouyuanbo pushed a commit to zhouyuanbo/video_player_2.6.1 that referenced this issue Jun 1, 2023
@stuartmorgan
[video_player] Add 5.0 interface support (#4627)
8ca0f5a 
Allows the other video_player packages to use either 4.x or 5.x of
`video_player_platform_interface`, since the only breaking change
doesn't affect them (as it just removes test code that they no longer
use). This allows clients of `video_player` to no longer have a
transitive dependency on test packages, but doesn't create any version
lock with any unendorsed implementations that might exist.

Fixes flutter/flutter#83562
@nt4f04uNd nt4f04uNd added the r: fixed Issue is closed as already fixed in a newer version label Jun 12, 2023
@flutter-triage-bot flutter-triage-bot bot added the package flutter/packages repository. See also p: labels. label Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@stuartmorgan stuartmorgan

Labels
p: video_player The Video Player plugin P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. r: fixed Issue is closed as already fixed in a newer version
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[video_player] Add 5.0 interface support stuartmorgan/plugins
6 participants
@stuartmorgan @tjarvstrand @Hixie @gaaclarke @nt4f04uNd @TahaTesser