Repository navigation
test: Use color checks that tolerate floating-point errors - #949
Conversation
chrisbobbe
added
the
integration review
gnprice
left a comment
gnprice
left a comment
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)
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.
3 participants
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)