Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Oct 30, 2024

Instead of completely private. This has been broken for Impeller for years, which shows how much this method is getting used.

Fixes #130461

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Oct 30, 2024
@zanderso
Copy link
Member

Nice. Is it possible to test this on CI?


// Check PNG header.
expect(bytes.sublist(0, 8), <int>[137, 80, 78, 71, 13, 10, 26, 10]);
});
Copy link
Member

Choose a reason for hiding this comment

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

Does this also need Timeout.none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm currently investigating this, but it looks like none of these driver tests are running anywhere.

final Uint8List data = (await driver.screenshot(format: ScreenshotFormat.rawStraightRgba)) as Uint8List;

expect(data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3], 0xFF0000FF);
}, timeout: Timeout.none);
Copy link
Member

Choose a reason for hiding this comment

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

A comment about why Timeout.none is needed here will probably be helpful to future readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its only needed to satisfy a lint check :)

@jonahwilliams jonahwilliams marked this pull request as ready for review October 31, 2024 16:32
@override
Future<List<int>> screenshot() async {
Future<List<int>> screenshot({ScreenshotFormat format = ScreenshotFormat.png}) async {
assert(format == ScreenshotFormat.png, 'Web Driver only supports PNG screenshot format');
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this an ArgumentError (yeah all tests use assertions but driver can be used outside of tests).

Future<List<int>> screenshot({ScreenshotFormat format = ScreenshotFormat.png}) async {
assert(format == ScreenshotFormat.png, 'Web Driver only supports PNG screenshot format');
if (format != ScreenshotFormat.png) {
throw ArgumentError('Web Driver only supports PNG screenshot format');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ArgumentError.value so the user sees what format was providwed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 31, 2024

auto label is removed for flutter/flutter/157888, due to - The status or check suite Mac_x64 plugin_lint_mac has failed. Please fix the issues identified (or deflake) before re-applying this label.

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2024
@auto-submit auto-submit bot merged commit 1050959 into flutter:master Oct 31, 2024
154 checks passed
@jonahwilliams jonahwilliams deleted the use_public_api branch October 31, 2024 21:32
auto-submit bot pushed a commit to flutter/engine that referenced this pull request Oct 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 1, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 1, 2024
flutter/flutter@0fe6153...f86b777

2024-11-01 [email protected] Roll Packages from 7cc1caa to 796afa3 (15 revisions) (flutter/flutter#158003)
2024-11-01 [email protected] Marks Linux_pixel_7pro service_extensions_test to be flaky (flutter/flutter#157853)
2024-11-01 [email protected] Roll Flutter Engine from 0a0d5c9be6ff to 3a090b46dd35 (1 revision) (flutter/flutter#157994)
2024-11-01 [email protected] Roll Flutter Engine from bacc5e1e73b7 to 0a0d5c9be6ff (3 revisions) (flutter/flutter#157991)
2024-11-01 [email protected] Add test for `interactive_viewer.transformation_controller.0.dart` (flutter/flutter#157986)
2024-11-01 [email protected] Roll Flutter Engine from d7e928911ac2 to bacc5e1e73b7 (1 revision) (flutter/flutter#157982)
2024-11-01 [email protected] Add test for `notification.0.dart` (flutter/flutter#157909)
2024-11-01 [email protected] performance: Override .elementAt in CachingIterable (flutter/flutter#152477)
2024-11-01 [email protected] Roll Flutter Engine from cd46383cd55e to d7e928911ac2 (4 revisions) (flutter/flutter#157978)
2024-11-01 [email protected] Roll Flutter Engine from bb77cf867aef to cd46383cd55e (11 revisions) (flutter/flutter#157972)
2024-11-01 [email protected] Add a warning/additional handlers for parsing`synthetic-package`. (flutter/flutter#157934)
2024-10-31 [email protected] Roll Flutter Engine from f2154ef3e31c to bb77cf867aef (6 revisions) (flutter/flutter#157960)
2024-10-31 [email protected] Renames `injectBuildTimePluginFilesForWebPlatform` and removes unused named parameter. (flutter/flutter#157944)
2024-10-31 [email protected] [flutter_driver] use mostly public screenshot API. (flutter/flutter#157888)
2024-10-31 [email protected] Made insetPadding configurable for Date Picker Dialog (flutter/flutter#155651)
2024-10-31 [email protected] Fix showSnackBar can't access useMaterial3 from the theme (flutter/flutter#157707)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Impeller] Add devicelab integration tests for all desktop backends.

3 participants