Skip to content

Conversation

@b-luk
Copy link
Contributor

@b-luk b-luk commented Nov 21, 2025

The benchmarks were added in #178690, but that PR didn't update the .ci.yaml and TESTOWNERS files.

@b-luk b-luk requested a review from flar November 21, 2025 01:12
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds CI configuration in .ci.yaml and ownership information in TESTOWNERS for new arc macrobenchmark performance tests. The changes correctly add the new test targets. My review includes suggestions to improve maintainability by reducing duplication in the YAML configuration and by sorting the new entries in the TESTOWNERS file.

Comment on lines 3556 to 3591
- name: Linux_pixel_7pro draw_arcs_all_fill_styles_perf__timeline_summary
recipe: devicelab/devicelab_drone
presubmit: false
timeout: 60
properties:
tags: >
["devicelab", "android", "linux", "pixel", "7pro"]
task_name: draw_arcs_all_fill_styles_perf__timeline_summary

- name: Mac_ios draw_arcs_all_fill_styles_perf_ios__timeline_summary
recipe: devicelab/devicelab_drone
presubmit: false
timeout: 60
properties:
tags: >
["devicelab", "ios", "mac"]
task_name: draw_arcs_all_fill_styles_perf_ios__timeline_summary

- name: Linux_pixel_7pro draw_arcs_all_stroke_styles_perf__timeline_summary
recipe: devicelab/devicelab_drone
presubmit: false
timeout: 60
properties:
tags: >
["devicelab", "android", "linux", "pixel", "7pro"]
task_name: draw_arcs_all_stroke_styles_perf__timeline_summary

- name: Mac_ios draw_arcs_all_stroke_styles_perf_ios__timeline_summary
recipe: devicelab/devicelab_drone
presubmit: false
timeout: 60
properties:
tags: >
["devicelab", "ios", "mac"]
task_name: draw_arcs_all_stroke_styles_perf_ios__timeline_summary

Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There is significant duplication across these four new test targets. To improve maintainability and adhere to the 'Avoid duplicating state' principle from the style guide1, consider using YAML anchors and aliases to define common properties. This would make the configuration cleaner and easier to manage in the future.

Style Guide References

Footnotes

  1. The style guide recommends avoiding duplication. Using YAML anchors for repeated configuration blocks would align with this principle.

Comment on lines +117 to +118
/dev/devicelab/bin/tasks/draw_arcs_all_fill_styles_perf__timeline_summary.dart @b-luk @flutter/engine
/dev/devicelab/bin/tasks/draw_arcs_all_stroke_styles_perf__timeline_summary.dart @b-luk @flutter/engine
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For better maintainability and consistency, please keep test owner entries sorted alphabetically within each section. These new entries should be placed after line 114 (...complex_layout_scroll_perf_impeller_gles__timeline_summary.dart). While I see other entries are also out of order, it would be good to place new entries correctly.

Comment on lines +239 to +240
/dev/devicelab/bin/tasks/draw_arcs_all_fill_styles_perf_ios__timeline_summary.dart @b-luk @flutter/engine
/dev/devicelab/bin/tasks/draw_arcs_all_stroke_styles_perf_ios__timeline_summary.dart @b-luk @flutter/engine
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For better maintainability and consistency, please keep test owner entries sorted alphabetically within each section. These new entries should be placed before line 236 (...static_path_stroke_tessellation_perf_ios__timeline_summary.dart).

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

I think the alphabetical suggestion from the bot makes sense. I don't know that we've embraced the de-duplication suggestion in that file, though, yet...

@b-luk
Copy link
Contributor Author

b-luk commented Nov 21, 2025

I think alphabetizing would be good. But the file isn't currently sorted, so I can't alphabetize these new entries without reordering the entire file. I think that's out of scope of this PR. I'll file a new issue about this.

@b-luk b-luk added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Nov 21, 2025
Merged via the queue into flutter:master with commit 315c213 Nov 21, 2025
152 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2025
@b-luk b-luk deleted the ciyamlforarcmacrobenchmarks branch November 21, 2025 19:25
github-merge-queue bot pushed a commit that referenced this pull request Nov 22, 2025
Sorting these entries was suggested by the gemini-code-assist bot in
another PR which modified the TESTOWNERS file: #178891 (see comments at
#178891 (comment)
and
#178891 (comment))

This is a follow-up PR to perform the sorting. We don't want to do it in
the PR where it was suggested #178891 because sorting the entries is out
of scope of that PR's goal.
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 22, 2025
flutter/flutter@d438df3...c8cfb2b

2025-11-22 [email protected] Revert "[ios] Dynamic Content Resizing" (flutter/flutter#178958)
2025-11-22 [email protected] Make sure that a SearchAnchor doesn't crash in 0x0 environment (flutter/flutter#177749)
2025-11-22 [email protected] Make sure that a Stepper doesn't crash in 0x0 environment (flutter/flutter#178068)
2025-11-22 [email protected] Remove deprecated `activeColor` in `switch.0.dart` example (flutter/flutter#178293)
2025-11-22 [email protected] Make sure that a UserAccountsDrawerHeader doesn't crash in 0x0 enviro… (flutter/flutter#178516)
2025-11-22 [email protected] Make sure that a TimePickerDialog doesn't crash in 0x0 environment (flutter/flutter#178451)
2025-11-22 [email protected] Make sure that a CupertinoLinearActivityIndicator doesn't crash in 0x… (flutter/flutter#178566)
2025-11-22 [email protected] Make sure that a CupertinoTabBar doesn't crash in 0x0 environment (flutter/flutter#178613)
2025-11-22 [email protected] Make sure that a CupertinoContextMenu doesn't crash in 0x0 environment (flutter/flutter#178722)
2025-11-22 [email protected] Sort entries in TESTOWNERS (flutter/flutter#178939)
2025-11-22 [email protected] [ios] Dynamic Content Resizing (flutter/flutter#177410)
2025-11-21 [email protected] Roll Skia from dc88b21ce7d2 to 3018c3053490 (2 revisions) (flutter/flutter#178947)
2025-11-21 [email protected] Manually roll material_color_utilities (flutter/flutter#170000)
2025-11-21 [email protected] Roll Fuchsia Linux SDK from Y-cMdgKy3d6EnibWR... to 4ul9jvZ7jnDGIjtCD... (flutter/flutter#178934)
2025-11-21 [email protected] Roll Dart SDK from c788b6a7aefd to 5af71c736b0a (1 revision) (flutter/flutter#178932)
2025-11-21 [email protected] Roll Skia from c588bb60d5da to dc88b21ce7d2 (2 revisions) (flutter/flutter#178933)
2025-11-21 [email protected] Update .ci.yaml and TESTOWNERS for arc macrobenchmark tests (flutter/flutter#178891)
2025-11-21 [email protected] Use interactive mode with `devicectl` to redirect stdout (flutter/flutter#178405)
2025-11-21 [email protected] Update .ci.yaml in flutter/flutter to use either macOS 15.5 or macOS … (flutter/flutter#178666)
2025-11-21 [email protected] Roll Skia from d4e9d2873bfd to c588bb60d5da (1 revision) (flutter/flutter#178928)
2025-11-21 [email protected] Roll Packages from b1e2fb0 to e67b6be (7 revisions) (flutter/flutter#178927)

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
mboetger pushed a commit to mboetger/flutter that referenced this pull request Dec 2, 2025
…178891)

The benchmarks were added in flutter#178690, but that PR didn't update the
.ci.yaml and TESTOWNERS files.
mboetger pushed a commit to mboetger/flutter that referenced this pull request Dec 2, 2025
Sorting these entries was suggested by the gemini-code-assist bot in
another PR which modified the TESTOWNERS file: flutter#178891 (see comments at
flutter#178891 (comment)
and
flutter#178891 (comment))

This is a follow-up PR to perform the sorting. We don't want to do it in
the PR where it was suggested flutter#178891 because sorting the entries is out
of scope of that PR's goal.
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
…178891)

The benchmarks were added in flutter#178690, but that PR didn't update the
.ci.yaml and TESTOWNERS files.
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
Sorting these entries was suggested by the gemini-code-assist bot in
another PR which modified the TESTOWNERS file: flutter#178891 (see comments at
flutter#178891 (comment)
and
flutter#178891 (comment))

This is a follow-up PR to perform the sorting. We don't want to do it in
the PR where it was suggested flutter#178891 because sorting the entries is out
of scope of that PR's goal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants