Skip to content

Made some pixel tests fuzzy #154680

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.

Sign up for GitHub

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

Merged
auto-submit merged 3 commits into from
Sep 5, 2024
Merged

Made some pixel tests fuzzy #154680

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1010a8
Made some pixel tests fuzzy
gaaclarke Sep 5, 2024
0738df7
tweak
gaaclarke Sep 5, 2024
45a9df2
parens
gaaclarke Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions packages/flutter/test/painting/decoration_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:ui' as ui show ColorFilter, Image;
import 'dart:ui' as ui show Color, ColorFilter, Image;

import 'package:fake_async/fake_async.dart';
import 'package:flutter/foundation.dart';
Expand All @@ -14,6 +14,36 @@ import '../image_data.dart';
import '../painting/mocks_for_image_cache.dart';
import '../rendering/rendering_tester.dart';

/// Positive result if the colors would be mapped to the same argb8888 color.
class _ColorMatcher extends Matcher {
_ColorMatcher(this._target);

final ui.Color _target;

@override
Description describe(Description description) {
return description.add('matches "$_target"');
}

@override
bool matches(dynamic item, Map<dynamic, dynamic> matchState) {
if (item is ui.Color) {
return item.colorSpace == _target.colorSpace &&
(item.a - _target.a).abs() <= (1 / 255) &&
(item.r - _target.r).abs() <= (1 / 255) &&
(item.g - _target.g).abs() <= (1 / 255) &&
(item.b - _target.b).abs() <= (1 / 255);
} else {
return false;
}
}

}

Matcher _matchesColor(ui.Color color) {
return _ColorMatcher(color);
}

class TestCanvas implements Canvas {
final List<Invocation> invocations = <Invocation>[];

Expand Down Expand Up @@ -326,7 +356,7 @@ void main() {
expect(call.positionalArguments[3], isA<Paint>());
final Paint paint = call.positionalArguments[3] as Paint;
expect(paint.colorFilter, colorFilter);
expect(paint.color, const Color(0x7F000000)); // 0.5 opacity
expect(paint.color, _matchesColor(const Color(0x7F000000))); // 0.5 opacity
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only failure? if so, it looks like all its doing is testing for 0.5 opacity. You could query that directly

Copy link
Member Author

@gaaclarke gaaclarke Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the only one that showed up 🤞 The next PR is going to be the one that's probably going to break a lot of stuff when I update things like lerp. We may need to put this color matcher somewhere else where it can be shared for that.

You mean something like expect(paint.color.a, closeTo(0.5))? It seems likely that was the intent of the author. I'd rather stick to what is actually asserted.

Copy link
Contributor

@jonahwilliams jonahwilliams Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean?

Copy link
Member Author

@gaaclarke gaaclarke Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like all its doing is testing for 0.5 opacity.

I'm saying that may have been their intention but it is actually asserting a color so it's safer to just keep it asserting the same thing that is written. If the goal is to avoid having to have a color matcher, we'll likely need one anyway to land flutter/engine#54981

Copy link
Contributor

@jonahwilliams jonahwilliams Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kk

expect(paint.filterQuality, FilterQuality.high);
expect(paint.isAntiAlias, true);
expect(paint.invertColors, isTrue);
Expand Down