Skip to content

Adds devicelab test documentation#188670

Open
gaaclarke wants to merge 1 commit into
flutter:masterfrom
gaaclarke:doc-device-lab
Open

Adds devicelab test documentation#188670
gaaclarke wants to merge 1 commit into
flutter:masterfrom
gaaclarke:doc-device-lab

Conversation

@gaaclarke

Copy link
Copy Markdown
Member

fixes #188640

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Jun 27, 2026

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

Copy link
Copy Markdown
Contributor

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 introduces a new test file devicelab_descriptions_validation_test.dart to validate that all DeviceLab tasks are documented with complete descriptions in DESCRIPTIONS.md, and updates dev/devicelab/README.md to link to the descriptions file. Feedback suggests throwing a StateError instead of calling fail() outside of a test context when the repository root is not found, and instantiating the RegExp object outside of the loop to improve performance.

Comment on lines +29 to +33
if (current.path == current.parent.path) {
fail(
'Could not find flutter repository root (${io.Directory.current.path} -> ${current.path})',
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Calling fail() from package:test outside of a test context (during the script initialization phase) is not recommended, as fail() is designed to be used within a running test. Instead, throw a StateError or another standard exception to signal that the repository root could not be found.

      if (current.path == current.parent.path) {
        throw StateError(
          'Could not find flutter repository root (${io.Directory.current.path} -> ${current.path})',
        );
      }

Comment on lines +42 to +46
String? currentTask;

for (final String line in descriptionsFile.readAsLinesSync()) {
if (line.startsWith('### [')) {
final RegExpMatch? match = RegExp(r'### \[([^\]]+)\]').firstMatch(line);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instantiating a RegExp object inside a loop can lead to unnecessary overhead and performance degradation, especially when parsing larger files. It is more efficient to define the regular expression once outside the loop.

  String? currentTask;
  final RegExp taskPattern = RegExp(r'### \[([^\]]+)\]');

  for (final String line in descriptionsFile.readAsLinesSync()) {
    if (line.startsWith('### [')) {
      final RegExpMatch? match = taskPattern.firstMatch(line);

@Piinks Piinks added the team-infra Owned by Infrastructure team label Jun 29, 2026
@flutter-dashboard

Copy link
Copy Markdown

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD team-infra Owned by Infrastructure team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

document devicelab tests

2 participants