Skip to content

Clean up hover test.#9826

Open
kenzieschmoll wants to merge 6 commits intoflutter:masterfrom
kenzieschmoll:tooltipfollowup
Open

Clean up hover test.#9826
kenzieschmoll wants to merge 6 commits intoflutter:masterfrom
kenzieschmoll:tooltipfollowup

Conversation

@kenzieschmoll
Copy link
Copy Markdown
Member

A follow up to #9823 to clean up the test.

@kenzieschmoll kenzieschmoll requested a review from a team as a code owner May 8, 2026 02:18
@kenzieschmoll kenzieschmoll requested review from bkonyi and removed request for a team May 8, 2026 02:18
@kenzieschmoll kenzieschmoll requested a review from srawlins May 8, 2026 02:18
Copy link
Copy Markdown
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 refactors the HoverCard component and its associated tests by exposing internal constants like hoverMargin and hoverDelay for testing purposes. It also cleans up hover_positioning_test.dart by replacing hardcoded dimensions and durations with named constants. A review comment identifies a remaining magic number in the tests that should be calculated relative to the window height and margin to improve robustness and adhere to the style guide.

Comment on lines +143 to +144
// Clamps strictly at y = 274.0 because of dynamic height containing title/divider.
expect(positionWithTitle.dy, equals(274.0));
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

[CONCERN] This test uses a hardcoded magic number 274.0 for the expected position. Since the PR aims to clean up the test and already introduces constants like _windowHeight and makes hoverMargin public, this expectation should be calculated relative to those constants. This makes the test more robust against changes to the window size or margin values. Additionally, using the actual height of the rendered box ensures the test correctly verifies that the bottom of the card is positioned at the expected margin from the window edge.

    expect(
      positionWithTitle.dy + renderBoxWithTitle.size.height,
      equals(_windowHeight - hoverMargin),
    );
References
  1. Strict avoidance of raw values in UI (use named constants) and general code quality (DRY, Meaningful Naming). (link)

Copy link
Copy Markdown
Contributor

@srawlins srawlins left a comment

Choose a reason for hiding this comment

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

Love it

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants