Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Add API consistency test #32408

Merged
merged 1 commit into from
Apr 7, 2022
Merged

Add API consistency test #32408

merged 1 commit into from
Apr 7, 2022

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Apr 4, 2022

Several enumerations need to be kept consistent between our native and
web dart:ui implementations and the embedder API implementation. There
are comments above each enum/class reminding SDK developers to keep
these in sync, but these frequently get out of sync.

This adds a test that verifies consistency between the identifiers in
these classes/enums to prevent them getting out of sync.

Issue: flutter/flutter#101217

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@cbracken cbracken requested review from Hixie, zanderso and dnfield April 4, 2022 17:48
@cbracken cbracken mentioned this pull request Apr 4, 2022
Merged
8 tasks
@cbracken
Copy link
Member Author

cbracken commented Apr 4, 2022

This is expected to fail until the following patches are landed:

@cbracken
Copy link
Member Author

cbracken commented Apr 4, 2022

Looks like I missed a couple. I'll add the check for the internal SemanticsFlags etc. enums from semantics_node.h and see if I can get the Java one added for Android.

@cbracken cbracken requested a review from chinmaygarde April 4, 2022 19:14
@cbracken
Copy link
Member Author

cbracken commented Apr 4, 2022

Adding @chinmaygarde since as written, this test will require the embedder API to be updated whenever public dart:ui enums are updated. That increases the odds that if we deprecate and remove one of these values from dart:ui, that someone might be tempted to remove it from the embedder API rather than update the test to support deprecated values remaining in the embedder API to keep our compatibility guarantees.

cbracken added a commit that referenced this pull request Apr 4, 2022
@cbracken
Fix inconsistent enum/class private member naming (#32409)
ac21195 
Classes that model enums in dart:ui typically name fields modelling the
enum values as `_kFooIndex`. This associated value matches the enum
value from embedder.h.

In #32408 we add a test that
verifies that dart:ui classes model the same set of values in the native
implementation, the web_ui implementation, and the embedder API. Testing
this is much simpler if we use consistent naming for all enum values.

Issue: #32408
zanderso
zanderso reviewed Apr 5, 2022
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

Please add an entry to https://github.com/flutter/engine/blob/main/tools/pub_get_offline.py#L17

@cbracken cbracken added the Work in progress (WIP) Not ready (yet) for review! label Apr 5, 2022
This was referenced Apr 6, 2022
Merged
Merged
Merged
@cbracken cbracken removed the Work in progress (WIP) Not ready (yet) for review! label Apr 7, 2022
@cbracken
Sync Android AccessibilityFeature enum and dart:ui
30378c9 
This brings the Android accessibility bridge AccessibilityFeature enum
back in sync with the `AccessibilityFeatures` class in dart:ui, defined
in lib/ui/window.dart

Keeping these enums in sync aids in automated API consistency checking.

Issue: flutter/flutter#101217
@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 7, 2022
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Web Framework tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 7, 2022
@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 7, 2022
@fluttergithubbot fluttergithubbot merged commit 816e3d3 into flutter:main Apr 7, 2022
@cbracken cbracken deleted the enum-check branch April 7, 2022 23:08
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 7, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 7, 2022
@engine-flutter-autoroll
816e3d3 Add API consistency test (flutter/engine#32408)
7bbc538
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Hixie Hixie Awaiting requested review from Hixie

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

2 more reviewers

@zanderso zanderso zanderso left review comments

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cbracken @fluttergithubbot @zanderso @dnfield