Sitelet https://github.com/flutter/flutter/pull/189800
Skip to content

Scope SemanticsTester per test in scrollable_semantics_test - #189800

Merged
auto-submit[bot] merged 1 commit into
flutter:masterfrom
mbcorona:cleanup-189799-scoped-semantics-tester
Jul 29, 2026
Merged

Scope SemanticsTester per test in scrollable_semantics_test#189800
auto-submit[bot] merged 1 commit into
flutter:masterfrom
mbcorona:cleanup-189799-scoped-semantics-tester

Conversation

@mbcorona

@mbcorona mbcorona commented Jul 21, 2026

Copy link
Copy Markdown
Member

The test file used a single top-level SemanticsTester semantics; shared across all tests. That is an anti-pattern: state can bleed between tests if one fails to dispose it.

This scopes the SemanticsTester locally within each test instead, removing the shared top-level variable. No behavior change to the tests themselves.

Fixes #189799

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Jul 21, 2026
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jul 21, 2026
View reviewed changes

@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 refactors scrollable_semantics_test.dart by removing the shared semantics variable and declaring it locally within each test case. Feedback recommends using addTearDown(semantics.dispose) immediately after instantiating SemanticsTester in all tests to ensure proper cleanup on failure, and suggests changing the single occurrence of var to final for consistency.

Comment thread packages/flutter/test/widgets/scrollable_semantics_test.dart

testWidgets('scrollable exposes the correct semantic actions', (WidgetTester tester) async {
semantics = SemanticsTester(tester);
final semantics = SemanticsTester(tester);

@gemini-code-assist gemini-code-assist Bot Jul 21, 2026

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.

high

To robustly prevent semantics state from bleeding between tests when a test fails, we should register the disposal using addTearDown(semantics.dispose); immediately after instantiating the tester. If an assertion fails, the rest of the test body is skipped, so any manual semantics.dispose() at the end of the test will not be executed. Using addTearDown ensures that dispose() is called even on test failure. Please apply this pattern to all other tests in this file where SemanticsTester is instantiated.

Suggested change
final semantics = SemanticsTester(tester);
final semantics = SemanticsTester(tester);
addTearDown(semantics.dispose);

@mbcorona mbcorona Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

addTearDown doesn't work for SemanticsTester: the framework's _verifySemanticsHandlesWereDisposed() runs inside runTest's finally block, before addTearDown callbacks fire

@Piinks Piinks Jul 28, 2026

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.

Oh interesting! Is this a bug? When is it disposed then?

@Piinks Piinks Jul 28, 2026

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.

Ah I see it is disposed at the end of the test. Maybe this is a bug though:

the framework's _verifySemanticsHandlesWereDisposed() runs inside runTest's finally block, before addTearDown callbacks fire

Should we file an issue to see?

Comment thread packages/flutter/test/widgets/scrollable_semantics_test.dart

// Semantics on
semantics = SemanticsTester(tester);
var semantics = SemanticsTester(tester);

@gemini-code-assist gemini-code-assist Bot Jul 21, 2026

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

Use final instead of var for consistency with other tests in this file, as the semantics variable is not reassigned. Additionally, register its disposal using addTearDown to ensure it is cleaned up even if the test fails.

Suggested change
var semantics = SemanticsTester(tester);
final semantics = SemanticsTester(tester);
addTearDown(semantics.dispose);

@mbcorona mbcorona Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

var is intentional — the toggle test reassigns semantics (dispose → recreate to turn semantics off then on again), so final won't compile here.

@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: scrolling Viewports, list views, slivers, etc. labels Jul 21, 2026
@mbcorona

mbcorona commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jul 21, 2026
View reviewed changes

@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 refactors "scrollable_semantics_test.dart" by removing the top-level "semantics" variable and declaring it locally within each test case. It is recommended to change "var semantics" to "final semantics" in the one test case where it was declared with "var" to maintain consistency and adhere to the Dart style guide.

Comment thread packages/flutter/test/widgets/scrollable_semantics_test.dart

// Semantics on
semantics = SemanticsTester(tester);
var semantics = SemanticsTester(tester);

@gemini-code-assist gemini-code-assist Bot Jul 21, 2026

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

For consistency with the other tests in this file and to adhere to the Dart style guide (which prefers final for local variables that are not reassigned), please use final instead of var here.

Suggested change
var semantics = SemanticsTester(tester);
final semantics = SemanticsTester(tester);
References
  1. Effective Dart: Style prefers declaring variables as final when they are not reassigned. (link)

@mbcorona
mbcorona requested a review from Piinks July 22, 2026 13:39
Piinks
Piinks approved these changes Jul 28, 2026
View reviewed changes

@Piinks Piinks 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.

LGTM!

@Piinks
Piinks force-pushed the cleanup-189799-scoped-semantics-tester branch from e4a1b5b to cc161d4 Compare July 28, 2026 22:10
@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 28, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Jul 29, 2026
Merged via the queue into flutter:master with commit 3656164 Jul 29, 2026
22 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 29, 2026
This was referenced Jul 29, 2026
Closed
Closed
Closed
Closed
Merged
Closed
Closed
Closed
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Jul 30, 2026
@engine-flutter-autoroll
Manual roll Flutter from 0f0246377b1c to c83f80b2367b (7 revisions) (#…
ff77bee 
…12318)

Manual roll requested by stuartmorgan@google.com

flutter/flutter@0f02463...c83f80b

2026-07-29 srawlins@google.com Use super-parameter in two_dimensional_utils (flutter/flutter#189957)
2026-07-29 brunocorona.alcantar@gmail.com Scope SemanticsTester per test in scrollable_semantics_test (flutter/flutter#189800)
2026-07-29 engine-flutter-autoroll@skia.org Roll Skia from d78865e708ad to 70733f74d415 (2 revisions) (flutter/flutter#190168)
2026-07-28 chris@bracken.jp iOS: Fix use-after-free race during shell teardown (flutter/flutter#190132)
2026-07-28 engine-flutter-autoroll@skia.org Roll Skia from 62442d6cf0ec to d78865e708ad (30 revisions) (flutter/flutter#190152)
2026-07-28 30870216+gaaclarke@users.noreply.github.com Made linter more robust to non-utf8 files (flutter/flutter#190012)
2026-07-28 engine-flutter-autoroll@skia.org Roll Packages from 6969329 to 3e63635 (6 revisions) (flutter/flutter#190142)

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 stuartmorgan@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) CICD Run CI/CD f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor scrollable_semantics_test.dart to avoid shared top-level SemanticsTester

2 participants

@mbcorona @Piinks