Skip to content

Reland Scribble mixin#115296

Closed
justinmc wants to merge 10 commits intomasterfrom
revert-115146-revert-114698-revert-114647-revert-104128-scribble-mixin
Closed

Reland Scribble mixin#115296
justinmc wants to merge 10 commits intomasterfrom
revert-115146-revert-114698-revert-114647-revert-104128-scribble-mixin

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Nov 14, 2022 •
edited
Loading

Scribble mixin (#104128) has been reverted several times due to breaking Google tests, most recently in #115146. This PR is an attempt to reland it.

Also, it seems to have caused a performance regression: #114898

Before merging this reland, we should be sure that the test failure and the performance problem are fixed.

Google bug for last failure: https://buganizer.corp.google.com/issues/258924157

@justinmc justinmc self-assigned this Nov 14, 2022
@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Nov 14, 2022
@justinmc justinmc marked this pull request as draft November 14, 2022 18:49
@flutter-dashboard
Copy link

flutter-dashboard bot commented Nov 14, 2022

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

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.

@justinmc
Copy link
Contributor Author

justinmc commented Nov 16, 2022

Removing draft status in order to run Google tests.

@justinmc
Copy link
Contributor Author

justinmc commented Nov 16, 2022

I looked over the code again and didn't see anything suspicious for the performance regression... I have not rerun the benchmark though.

@justinmc justinmc marked this pull request as ready for review November 17, 2022 19:13
CaseyHillers
CaseyHillers approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@CaseyHillers CaseyHillers left a comment

Choose a reason for hiding this comment

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

RSLGTM for triggering Google Testing

@moffatman moffatman mentioned this pull request Dec 23, 2022
Merged
8 tasks
@goderbauer
Copy link
Member

goderbauer commented Jan 3, 2023

(Triage): @justinmc Do you still have plans for this one?

@justinmc
Copy link
Contributor Author

justinmc commented Jan 3, 2023

Yes, I'll take a closer look at the Google test failures this week.

@moffatman moffatman mentioned this pull request Jan 25, 2023
Closed
@jmagman
Copy link
Member

jmagman commented Jan 30, 2023

If this is still in progress, can we change the names back ASAP in a smaller PR to resolve #119180?

@moffatman in #119180 (comment)

Maybe the best way to fix is to first change the names back, then land the rest of the changes? That would be easiest to cherry-pick. @justinmc

@justinmc
Copy link
Contributor Author

justinmc commented Jan 31, 2023

Ah it didn't cross my mind that the engine PR would start breaking things since this was reverted. I'm trying to get this back up to speed.

@justinmc justinmc force-pushed the revert-115146-revert-114698-revert-114647-revert-104128-scribble-mixin branch from 58c4cae to 6ffc282 Compare January 31, 2023 18:22
@justinmc
Merge branch 'master' into revert-115146-revert-114698-revert-114647-…
879296f 
…revert-104128-scribble-mixin

Manually moved changed scribble test from text_input_test to scribble_test
@justinmc
Merge branch 'master' into revert-115146-revert-114698-revert-114647-…
89d1a41 
…revert-104128-scribble-mixin
@justinmc justinmc force-pushed the revert-115146-revert-114698-revert-114647-revert-104128-scribble-mixin branch from 6ffc282 to 89d1a41 Compare January 31, 2023 18:43
@justinmc justinmc mentioned this pull request Feb 1, 2023
Closed
@goderbauer
Copy link
Member

goderbauer commented Feb 1, 2023

(triage) Can this one be closed in favor of #119739?

justinmc added a commit to justinmc/flutter that referenced this pull request Feb 2, 2023
@justinmc
Squash PR flutter#115296. Reverts scribble mixin revert and updates w…
0243742 
…ith master.
@justinmc
Copy link
Contributor Author

justinmc commented Feb 2, 2023

Let me see if the tests pass on #119739 (I accidentally committed the diff file so I just pushed again). If it looks like this one has a problem then I'll close it.

@justinmc
Copy link
Contributor Author

justinmc commented Feb 6, 2023

Both PRs seem to have identical Google test failures. I will close one of the two PRs.

@justinmc justinmc mentioned this pull request Feb 9, 2023
Merged
@jmagman jmagman mentioned this pull request Feb 10, 2023
Closed
@flutter-dashboard
Copy link

flutter-dashboard bot commented Mar 2, 2023

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.

@goderbauer
Copy link
Member

goderbauer commented Mar 21, 2023

(triage) @justinmc Do you still have plans for this one?

@justinmc
Copy link
Contributor Author

justinmc commented Mar 27, 2023

@goderbauer Updated. I plan to return to this to try to fix the Google test failures.

@justinmc
Copy link
Contributor Author

justinmc commented May 30, 2023

I'm going to update this and make another attempt at seeing why it fails Google tests.

@github-actions github-actions bot removed the framework flutter/packages/flutter repository. See also f: labels. label May 30, 2023
@justinmc justinmc force-pushed the revert-115146-revert-114698-revert-114647-revert-104128-scribble-mixin branch from 6566f44 to b9fdb72 Compare June 1, 2023 17:29
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. a: test labels Jun 1, 2023
@justinmc justinmc force-pushed the revert-115146-revert-114698-revert-114647-revert-104128-scribble-mixin branch from b9fdb72 to d4e256e Compare June 1, 2023 17:30
@goderbauer goderbauer removed the a: test label Jun 2, 2023
@justinmc
Copy link
Contributor Author

justinmc commented Aug 31, 2023

Closing this as I won't have time to fix it for awhile. Hoping to come back and fix the Google failures at some point.

@justinmc justinmc closed this Aug 31, 2023
@guidezpl guidezpl deleted the revert-115146-revert-114698-revert-114647-revert-104128-scribble-mixin branch April 18, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@CaseyHillers CaseyHillers CaseyHillers approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@justinmc justinmc

Labels

a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems 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.

4 participants

@justinmc @goderbauer @jmagman @CaseyHillers