Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DisplayList] Fix assertions on DisplayList verbose comparison tests #54065

Merged

Conversation

flar
Copy link
Contributor

@flar flar commented Jul 23, 2024

Fixes 2 problems recently uncovered in the DisplayList verbose comparison test mechanism:

  • The verbose compare methods never asserted a test failure, relying on the caller to do so from their return value - but they also did not prompt the caller to check the return value. So a [[nodiscard]] is added to remind test writers that they need to assert on the return value
  • As a result of the above, some bad tests were recently added to the tree that were failing but did not assert a test failure. Now that the [[nodiscard]] is added, they failed to compile and had to have asserts added.

A secondary problem is that those non-failing tests were inadvertently cherry-picked from a reverted PR that is being reintroduced in incremental sections so as to avoid large scale golden image failures. The tests depend on parts of that PR that haven't been pulled forward yet (but will soon be) so those tests shouldn't have been added in the first place (and were failing, but not causing a gtest failure because of their missing asserts). They remain here, but their results are reversed to indicate the current state of affairs (they assert that the missing functionality isn't in place yet). Their assertions will be reverted when/as the missing functionality is pulled forward in a more incremental (responsible) way.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 23, 2024
@auto-submit auto-submit bot merged commit a1278eb into flutter:main Jul 24, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
Status: Done
2 participants