test: Use color checks that tolerate floating-point errors #949
Conversation
chrisbobbe
added
the
integration review
There was a problem hiding this comment.
Thanks for taking care of this!
The changes all look good, except that I think a Flutter deps bump is needed before adding the references to isSameColorSwatchAs. (The analyzer is failing for me locally because I have a Flutter tree from a few days ago.)
Some other work was needed outside what's described in that draft;
see:
https://github.com/flutter/flutter/issues/155113#issuecomment-2347374468
https://github.com/flutter/flutter/issues/155113#issuecomment-2356356254
I think this work just meant (a) using package:flutter_checks, and (b) those few lines adding a wrapper for isSameColorSwatchAs. Is that right or is there another piece I'm missing?
To ensure `isSameColorSwatchAs` is present; we'll use that in an upcoming commit. From flutter/flutter#155272.
Thanks lishaduck for creating these with us in mind! Discussion: zulip#232 (comment)
Done to follow a draft migration guide: https://github.com/flutter/website/blob/997c893ccec5fb15dd085a4d23fe8c43870c631b/src/content/release/breaking-changes/wide-gamut-framework.md for the proposed breaking change that raised flutter/flutter#155113 In our case we also needed to use `package:flutter_checks` and `package:legacy_checks` to wrap the legacy `Matcher`s `isSameColorAs` and `isSameColorSwatchAs` from `package:flutter_test`; see discussion of those: flutter/flutter#155113 (comment) flutter/flutter#155113 (comment)
chrisbobbe
force-pushed
the
pr-colors-checks
branch
from
3c87386 to
cdbe141
Compare
|
Yep! Revision pushed, PTAL. |
|
Thanks! Looks good — merging. |
| extension ColorSwatchChecks<T> on Subject<ColorSwatch<T>> { | ||
| /// package:checks-style wrapper for [flutter_matcher.isSameColorSwatchAs]. | ||
| void isSameColorSwatchAs(ColorSwatch<T> colorSwatch) { | ||
| legacyMatcher(flutter_matcher.isSameColorSwatchAs(colorSwatch)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Cool, I'm glad legacy_checks was useful!
I was worried I'd missed one for a second before I realized it was new. I know y'all follow the master channel, so, as I don't think I wrote down a flutter version support policy anywhere, I guess I'll just note here that I don't want to publish anything that makes it unnecessarily require a "nightly". I'm happy to accept PRs once they land in stable, but you'll probably still need to maintain a few of these.
There was a problem hiding this comment.
Sure, that approach makes sense.
Thanks to legacyMatcher, any individual wrappers like this one should be pretty cheap to write, like this one was.
Done to follow a draft migration guide:
https://github.com/flutter/website/blob/997c893ccec5fb15dd085a4d23fe8c43870c631b/src/content/release/breaking-changes/wide-gamut-framework.md
for the proposed breaking change that raised
flutter/flutter#155113
Some other work was needed outside what's described in that draft;
see:
flutter/flutter#155113 (comment)
flutter/flutter#155113 (comment)