Zulip test needs to migrate color assertions

@chrisbobbe

After flutter-team-archive/engine#54981 landed we experienced customer testing failures in Zulip so it had to be reverted. Now that dart:ui color is defined as floating-point numbers the isSameColorAs should be used for testing Color equality in tests.

Here's an example where I had to do that in the framework: https://github.com/flutter/flutter/pull/154752/files#diff-5e57f7f8adbaec70bf92aeb1da7f5b86d3f3f7b1239739f76b50615f09187015R197

This one is tricky because it looks like the difference in the floating point numbers is < 0.0001 which means it doesn't show up in the print statements. I'm not sure if there is a good remedy for that.

Please update the asserts or the customer test will be disabled in 72 hours (as per our SLA).

Thanks @chrisbobbe, sorry this popping up now. FWIW this is my last planned change to dart:ui Color for this feature.

I looked into queuing up this change for you but you are using package:checks which doesn't seem to support matchers. I'll let you figure out what you want to do about it. It seems like you have to make your own extension method to match the behavior of isSameColorAs

@gaaclarke

Hi @gaaclarke, thanks for the note.

Before there's any talk of disabling anything, the way customer testing works is that the stage before that is the breaking-change process:
https://github.com/flutter/tests#if-a-test-is-broken
https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes

Can you link to the draft migration guide for this breaking change? That'll help us with migrating our code. The draft migration guide is part of step 3 in the breaking-change process, which comes before migrating clients in step 4.

(And of course the migration guide will be essential for helping the broader ecosystem of Flutter users migrate — which is the point of the breaking-change process and of the customer-tests system.)

BTW the 72-hour SLA is on being responsive, not on updating the tests.

For this change I think that won't be a major distinction in practice, because it sounds like the migration will be relatively simple. (It's Thursday afternoon now, though, so if there are some wrinkles it might go to early next week.)

But I wanted to clarify that point because when a migration gets complex, the difference is important: if the migration takes time because it's complex, then it takes time. The customer tests exist to provide feedback to us as the Flutter team about how our changes affect users of Flutter; if a migration is complex for one of the customer tests, then it'll probably be complex for many other Flutter users.

Yes, sorry Greg I had talked with your team previously with this change so I thought you were all current. The breaking change guide has already been published. You'll find it here: https://github.com/flutter/website/blob/main/src/content/release/breaking-changes/wide-gamut-framework.md

It's not going to say anything about migrating package:check. It does not say anything about the equality of Color changing. It may be worthwhile adding that now that we know that is possible to come up.

I posted a PR to spell out the equality changes more explicitly. It's kind of a grey area since the API doesn't change but the change in semantics can break tests so better to mention it: https://github.com/flutter/website/pull/11132/files

Here's an easy change, looks like package:checks has a isCloseTo, you could do something like this if you don't want to make your own extension method:

diff --git a/test/widgets/emoji_reaction_test.dart b/test/widgets/emoji_reaction_test.dart
index a11d29c..a9dd024 100644
--- a/test/widgets/emoji_reaction_test.dart
+++ b/test/widgets/emoji_reaction_test.dart
@@ -246,7 +246,10 @@ void main() {
 
     await tester.pump(kThemeAnimationDuration * 0.4);
     final expectedLerped = EmojiReactionTheme.light().lerp(EmojiReactionTheme.dark(), 0.4);
-    check(backgroundColor('smile')).equals(expectedLerped.bgSelected);
+    check(backgroundColor('smile')!.a).isCloseTo(expectedLerped.bgSelected.a, 4e-3);
+    check(backgroundColor('smile')!.r).isCloseTo(expectedLerped.bgSelected.r, 4e-3);
+    check(backgroundColor('smile')!.g).isCloseTo(expectedLerped.bgSelected.g, 4e-3);
+    check(backgroundColor('smile')!.b).isCloseTo(expectedLerped.bgSelected.b, 4e-3);
     check(backgroundColor('tada')).equals(expectedLerped.bgUnselected);
 
     await tester.pump(kThemeAnimationDuration * 0.6);

Very helpful, thanks!

And I agree that added section about equality in the migration guide is a good idea.

flutter_checks makes it convenient to wrap and use Matchers from flutter_test that don't yet have counterparts in checks, including isSameColorAs(…):

import 'package:flutter_checks/flutter_checks.dart';
// […]
check(colorA).isSameColorAs(colorB);

The isSameColorAs(…) matcher is helpful for checks on ordinary Colors. But for ColorSwatches, we'd lose a handy way to compare two swatches for equality in all the members / "related colors" that make up each swatch.

Before, you could do this:

      // Compares all swatch members; see [ColorSwatch]'s `operator ==`.
      check(swatchA).equals(swatchB);

(The `operator ==` override on `ColorSwatch`)

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) {
      return true;
    }
    if (other.runtimeType != runtimeType) {
      return false;
    }
    return super == other
        && other is ColorSwatch<T>
        && mapEquals<T, Color>(other._swatch, _swatch);
  }

But now:

      // Compares all members, but thrown off by floating-point differences
      check(swatchA).equals(swatchB);
      
      // Possible because `ColorSwatch` extends `Color`,
      // and not sensitive to floating-point differences, but only looks at
      // each swatch's "primary" color, ignoring all its other members.
      check(swatchA).isSameColorAs(swatchB);

If there were a new Matcher for ColorSwatches that compared all the swatches' members using the logic of isSameColorAs (with a threshold), it would help us and other apps whose tests want to do that.

The isSameColorAs(…) matcher is helpful for checks on ordinary Colors. But for ColorSwatches, we'd lose a handy way to compare two swatches for equality in all the members / "related colors" that make up each swatch.

The equality problem's only going to arise if you are doing certain mathematical operations to calculate colors (like division). If you do something like this, it will still work:

const swatch = ColorSwatch<int>(0xff000000, {0, Color(0xff000000)});
final foo = Foo(swatch, swatchIndex: 0);
check(foo.color).equals(Color(0xff000000));

We're only running into an issue here because we are lerping the swatches. That seems somewhat niche, I don't know what the framework's threshold is for adding a new matcher. I think a good way forward would be to add it to your code if you find it helpful then open a framework PR that adds it and see what the framework team says.

For what it's worth the ColorSwatch is somewhat in my sight for elimination. It hasn't been investigated deeply but there is a hope we can make Color sealed at some point which would make subclassing Color verboten. In general if the classes in dart:ui are sealed we'll have more freedom to change them since people will be treating them as black boxes instead of relying on certain implementations.

edit: Well I don't mean eliminate ColorSwatch, but change it from being a Color subclass. In that case having a matcher would still be helpful.

Making ColorSwatch no longer subclass Color sounds pretty reasonable to me — it always felt a bit odd that it's a subclass. That'll be a breaking change of its own, I'm sure, so there'll be some migration work needed to make that happen too.

Lerping swatches is pretty common: it's something that any modern app can be expected to do if it uses swatches in the first place. Users expect apps to offer a dark theme as well as a light theme, and to animate smoothly between them if they've chosen in system settings for the device to switch back and forth on a schedule. So for example MaterialApp uses AnimatedTheme to lerp between the app's dark and light themes, and therefore lerp all the colors in the themes. If the app has swatches of its own, they're probably different between the themes and so will also get lerped.

More generally I think it's pretty normal to do mathematical operations on colors. For example division by 255 is right there in the Color class now :-). It's also common to transform colors to a different coordinate system like LCH, blend or adjust them there, and transform back, in order to get nice visual effects.

So before we ship a breaking change that makes color computations happen in floating point, we should have a good answer for how people can continue to write reasonable tests for their color-manipulating code, across all Flutter's affected APIs. The isSameColorAs matcher provides a good answer for comparing Color itself, but ColorSwatch needs a solution too.

Fortunately I think that shouldn't be complicated to do.

The key thing is that ColorSwatch will need to gain a getter like debugEntries => _swatch.entries, and/or debugKeys => _swatch.keys. As things stand, it's not possible for code outside that file to enumerate the keys of the swatch in order to drive a comparison.

Then it becomes straightforward to write a matcher. Because ColorSwatch is part of Flutter, and because its data is going to be in floating-point terms so that inexact comparisons are necessary, I think the right home for that matcher is in flutter_test, just like we have for Color and Offset and a number of other types.

I think it's kind of moot because the only thing that matters is what Color it manifests as. By asserting that ColorSwatch.lerp does something is recreating tests that already exist in the framework.

I put up a PR for accessing the keys to a ColorSwatch: #155262. It's not necessary for this. You'll have to pass in the list of keys in the matcher.

class _SwatchMatcher<T> extends Matcher {
  _SwatchMatcher(this._target, this._keys, this._threshold);
  final ColorSwatch<T> _target;
  final double _threshold;
  final Iterable<T> _keys;

  Iterable<T> _iterate<T>(ColorSwatch<T> swatch) sync* {
    int i = 0;
    for (final key in _keys) {
      yield key;
    }
  }

  @override
  bool matches(dynamic item, Map<dynamic, dynamic> matchState) {
    if (item is ColorSwatch<T>) {
      final targetIt = _iterate(_target).iterator;
      final itemIt = _iterate(item).iterator;
      bool targetMore = targetIt.moveNext();
      bool itemMore = itemIt.moveNext();
      while (targetMore && itemMore) {
        if (!matches(targetIt.current, itemIt.current, _threshold)) {
          return false;
        }      
        targetMore = targetIt.moveNext();
        itemMore = itemIt.moveNext();
      }
      return targetMore == itemMore;
    } else {
      return false;
    }
  }
}

I put up a PR for accessing the keys to a ColorSwatch: #155262.

Great, thanks.

As I said above, I think the right home for the matcher is in flutter_test. So there should be a flutter/flutter PR for that too, and then it'll be possible for Flutter users (including Zulip) to cleanly migrate.

Added .keys to ColorSwatch (#155262)

Adds ColorSwatch matcher (#155272)

#155272 adds a ColorSwatch matcher if you want to hook into that instead of writing your own. It doesn't recurse into ColorSwatch, which I don't think you're going to need.

@chrisbobbe

Excellent, thanks for adding that!

@chrisbobbe now has a PR migrating our tests to use that matcher (and isSameColorAs):

test: Use color checks that tolerate floating-point errors zulip/zulip-flutter#949

so I expect that'll be complete soon.

test: Use color checks that tolerate floating-point errors

Update zulip to use Color{,Swatch} asserts that tolerate float errors

The zulip-flutter PR has been merged, and I've just sent flutter/tests#409.

Update zulip to use Color{,Swatch} asserts that tolerate float errors (…

Reland: Update Color to do all calculations with floating point compo…

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.

Navigation Menu

Repository navigation

Zulip test needs to migrate color assertions #155113

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Zulip test needs to migrate color assertions #155113

Description

Activity

gaaclarke commented on Sep 12, 2024

gnprice commented on Sep 12, 2024

gnprice commented on Sep 12, 2024

gaaclarke commented on Sep 12, 2024

gaaclarke commented on Sep 12, 2024

gaaclarke commented on Sep 12, 2024

gnprice commented on Sep 12, 2024

chrisbobbe commented on Sep 12, 2024

chrisbobbe commented on Sep 12, 2024

gaaclarke commented on Sep 12, 2024

gnprice commented on Sep 14, 2024

gaaclarke commented on Sep 16, 2024

gaaclarke commented on Sep 16, 2024

gaaclarke commented on Sep 16, 2024

1 remaining item

gnprice commented on Sep 16, 2024

gaaclarke commented on Sep 17, 2024

gnprice commented on Sep 18, 2024

chrisbobbe commented on Sep 18, 2024

github-actions commented on Oct 3, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions