search

dart-lang / web_socket_channel

StreamChannel wrappers for WebSockets.
https://pub.dev/packages/web_socket_channel
BSD 3-Clause "New" or "Revised" License
424 stars 112 forks source link

Provide a fake WebSocketChannel implementation useful for tests #361

Open brianquinlan opened 6 months ago

brianquinlan commented 6 months ago
Contribution guidelines:
- See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) for general expectations for PRs. - Larger or significant changes should be discussed in an issue before creating a PR. - Contributions to our repos should follow the [Dart style guide](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
NotTsunami commented 6 months ago

Thanks for this. Small nit, but perhaps Mock would be a better name than Fake.

natebosch commented 6 months ago

Small nit, but perhaps Mock would be a better name than Fake.

My understanding of the definitions for different types of test doubles (and the one we use on the Dart team) is:

In usage many authors will feed it stubbed data, and will use the other side of the channel to verify interactions, but I don't think this class is a "Stub" or a "Mock" the way we would normally think of them. It's only used with the same API as the production class, so I think "Fake" is the right term to use.

It's definitely the case that historically on the Dart team, and in the Dart ecosystem in general, the work "Mock" has been used more frequently, and has been applied to instances that cannot verify interactions.

natebosch commented 6 months ago

Are we planning on landing this one in addition to a fake for WebSocket?

I think we could skip to landing the latter if we wanted.

NotTsunami commented 6 months ago

Small nit, but perhaps Mock would be a better name than Fake.

My understanding of the definitions for different types of test doubles (and the one we use on the Dart team) is:

  • "Test double" any instance that stands in place of the production implementation.
  • Mock - has some way to test expectations about the interaction with this object. Things like verify.
  • Stub - has some way to provide hardcoded return values. In many cases (and this is true in Dart Mockito) every Mock is also a Stub.
  • Fake - has test specific behavior implemented, acts sort of like the real instance, but with test environment considerations, like not using a real network.

In usage many authors will feed it stubbed data, and will use the other side of the channel to verify interactions, but I don't think this class is a "Stub" or a "Mock" the way we would normally think of them. It's only used with the same API as the production class, so I think "Fake" is the right term to use.

It's definitely the case that historically on the Dart team, and in the Dart ecosystem in general, the work "Mock" has been used more frequently, and has been applied to instances that cannot verify interactions.

Understood, thanks for the thorough explanation!

brianquinlan commented 6 months ago

Are we planning on landing this one in addition to a fake for WebSocket?

That was my thought.

I think we could skip to landing the latter if we wanted.

I think that getting people to move off of package:web_socket_channel will take a lot of time consider its high level of usage.

And package:web_socket_channel 3.0 did remove an easy path for creating fakes.

lishaduck commented 3 months ago

Is there anything in particular holding this up from merging? It's preventing package:build_runner from migrating to v3, which is in turn preventing migrating to package:web@^1 (as web_socket_channel@^2 doesn't declare support for web@^1). dependency_overrides can be used to work around it for now, but it'd sure be nice to be able to remove the override.

EDIT: Oh, CI is failing. Shouldn't be a bad fix. EDIT 2: It's fixed on the main branch, just needs a rebase.

brianquinlan commented 3 months ago

Is there anything in particular holding this up from merging? It's preventing package:build_runner from migrating to v3

Didn't I fix that in https://github.com/dart-lang/build/commit/43bd3f52ec84a22071f3a953a63030a354705758 ?

lishaduck commented 3 months ago

Didn't I fix that in dart-lang/build@43bd3f5 ?

Huh. Looks like it. Given that dart-lang/build#3678 was still open, I assumed it was blocked on this. Disregard. I must've misread some error messages from pub.