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

Find a package:checks analogue of flutter_test's widget-finder matchers #232

Closed
gnprice opened this issue Jul 20, 2023 · 13 comments
Closed
Labels
Milestone

Comments

@gnprice
Copy link
Member

gnprice commented Jul 20, 2023

In Flutter widget tests written with the venerable expect API, it's common to have expectations like

    expect(find.whatever(), findsOneWidget);

using the matchers findsOneWidget, findsNothing, and findsWidgets. See docs.

We're using package:checks instead, which is the projected, still-beta, successor to expect with a fancy type-safe API, so these "matchers" like findsOneWidget aren't available; and we don't currently have great analogues of them. One thing we currently often do in place of findsOneWidget is

    tester.widget(find.whatever());

which will indeed throw if there isn't exactly one such widget, but it's not great because it doesn't look like something intended as one of the checks that are the payload of the test — it looks like it's just part of the test's setup (and on top of that, isn't doing anything). A bit of discussion here: #207 (comment)

Shouldn't be hard to write appropriate checks in the package:checks API. We should do that, convert our existing tests, and also make sure there's a discussion upstream about there being good support for using package:checks with package:flutter_test before the former is rolled out as stable.

@lishaduck
Copy link

lishaduck commented Aug 15, 2024

Hello! I stumbled upon this looking to see if anyone else had written some widget-finder-with-checks tests.
I didn't see any here, so I went ahead and wrote my own: PHS-TSA/our_democracy@8731bbf#diff-fd2549beda0a78cbca448d282c13fd259d03f2bbeb229f978e77a66afe2df8d1

I hope it helps!
If you want to just plain vendor it, feel free to, it's 99% code from flutter_test (BSD-3; should already be depended on). If you need my consent, I'm fine with licensing the remaining 1% under Apache-2. I'd argue it's not enough to be copyrightable, but I'm not a lawyer, so... 🤷‍♂️

@gnprice
Copy link
Member Author

gnprice commented Aug 15, 2024

Very interesting, thanks!

I see the core of that implementation of findsOne is based on _FindsCountMatcher.matches from flutter_test.

I think the strategy I'd hope to be able to make work is to reuse the existing logic from there, rather than have to duplicate it. (There's a couple of thousands of lines in that matchers.dart file.) So for example something like:

  final matchState = {};
  if (!findsOne.matches(finder, matchState)) {
    return Rejection(
      // … call `findsOne.describe` and `findsOne.describeMismatch`, passing `matchState` to the latter …
    );
  }
  return null;

Even if that doesn't end up with something that would work well for a totally general implementation of the Matcher API, so long as it works well with the matchers that flutter_test actually supplies, that'd be a good outcome.

@lishaduck
Copy link

So you'd want an API like this?

check(find.whatever()).findsOneWidget();

or:

check(find.whatever()).legacyMatcher(findsOneWidget);

Or, I suppose, .findsOneWidget() as typesafe sugar for .legacyMatcher(findsOneWidget)?

Just figure I might as well make my impl work for y'all if I'm going to write it either way. I'm not particularly thrilled with the prospect of maintaining all of matcher 😀

@lishaduck
Copy link

lishaduck commented Aug 17, 2024

I couldn't figure out async matchers, but here's an impl for sync matchers, @gnprice.
You can pass async matchers, and they pass, but I think it's mostly a noop.
Tested by making sure that all my tests still pass.

import 'package:checks/context.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:matcher/src/expect/async_matcher.dart';

extension LegacyMatcher<T> on Subject<T> {
  void legacyMatcher(Matcher matcher) {
    context.expect(() => [], (value) {
      final matchState = <Object?, Object?>{};
      final match = matcher.matches(value, matchState);
      if (!match) {
        final description = matcher.describe(StringDescription());
        final mismatchDescription = matcher.describeMismatch(
          value,
          StringDescription(),
          matchState,
          false,
        );

        return Rejection(
          which: [description.toString()],
          actual: [mismatchDescription.toString()],
        );
      }
      return null;
    });
  }

  Future<void> legacyMatcherAsync(AsyncMatcher matcher) async {
    await context.expectAsync(() => [], (actual) async {
      final Object? match = await matcher.matchAsync(actual);

      // Do something with the match.
    });
  }
}

@lishaduck
Copy link

lishaduck commented Aug 18, 2024

Ok, wrote two basic packages, including async support. I'll probably publish it to pub at some time, any permissive license would work for y'all if it's external, right?
Given it's heavily based on matcher, I figure I'll stick with BSD-3, which should be compatible with apache.

@lishaduck
Copy link

@gnprice
Copy link
Member Author

gnprice commented Aug 22, 2024

Neat, thanks for building this!

So for the example in my original description above:

    expect(find.whatever(), findsOneWidget);

it looks like one would write:

    check(find.whatever()).findsOneWidget();

(And actually one would write findsOne instead of findsOneWidget, in both versions, since findsOne was introduced and deprecated findsOneWidget about a month after I originally filed this issue.)

That looks like a very clean usage pattern to me. I think the main remaining question I have is what the output looks like when things don't match: how does the output for some example compare between the old expect way and the new checks plus flutter_checks way?

That question comes to mind because the quality of output when a test fails is something that can make a big difference in the effort spent working out what a failed test is telling you (is it a bug in the change you just made? or is the test just brittle and needs updating? or something else?). It's therefore something both the expect and checks implementations spend significant effort on, and so I'm curious how well the matchers' output translates through the checks API.

And then I guess after that the only thing flutter_checks needs is to add similar wrappers for all the other matchers in that matchers.dart file in package:flutter_test. Which should be nice and straightforward given those handy wrappers in legacy_checks.

@gnprice
Copy link
Member Author

gnprice commented Aug 22, 2024

(I'll be offline next week and so might not reply promptly; but I'll see any comments here when I'm catching up after that, and respond then. Other Zulip folks might also reply before I do.)

@lishaduck
Copy link

Neat, thanks for building this!

You're welcome! 😁

So for the example in my original description above

Yup, exactly!

how does the output for some example compare between the old expect way and the new checks plus flutter_checks way?

Good question! Being perfectly honest, I forgot to add failures to the test suite so 🤷‍♂️ (I did test it manually, they work, but I didn't pay attention to more than that)
I'll try to find some time either tomorrow or next week to investigate and probably publish a new version with better formatting.

And then I guess after that the only thing flutter_checks needs is to add similar wrappers for all the other matchers in that matchers.dart file in package:flutter_test. Which should be nice and straightforward given those handy wrappers in legacy_checks.

Yeah, I only wrote what I needed for some boilerplate tests for a project I'm starting, which wasn't much 🙃
It shouldn't be too hard to write some more, I just didn't want to write them all if y'all had something different in mind api-wise (If I'm going to publish it, might as well make it usable), given that I didn't need them myself.

@gnprice
Copy link
Member Author

gnprice commented Aug 23, 2024

I'll try to find some time either tomorrow or next week to investigate and probably publish a new version with better formatting.

Sounds great, thanks!

Yeah, I only wrote what I needed for some boilerplate tests for a project I'm starting, which wasn't much 🙃
It shouldn't be too hard to write some more, I just didn't want to write them all if y'all had something different in mind api-wise

Yep, good strategy.

@lishaduck
Copy link

lishaduck commented Sep 4, 2024

I just got around to pushing and publishing [email protected]. It now supports all matchers from flutter_test.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Sep 17, 2024
Thanks lishaduck for creating these with us in mind! Discussion:
  zulip#232 (comment)
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Sep 18, 2024
Thanks lishaduck for creating these with us in mind! Discussion:
  zulip#232 (comment)
@gnprice
Copy link
Member Author

gnprice commented Sep 18, 2024

Thanks again @lishaduck for building that flutter_checks package!

We've now started using it, with 90c7b26 / #949.

I'll close this issue, as a package:checks analogue of those matchers from flutter_test has been found. I'll file a separate follow-up issue for converting our existing tests from patterns like a bare tester.widget(find.whatever()); to patterns like check(find.whatever()).findsOne();.

@gnprice
Copy link
Member Author

gnprice commented Sep 18, 2024

Filed #952 for that follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

2 participants