Skip to content

Remove workaround since #66006 is fixed - #106913

Closed
fzyzcjy wants to merge 2 commits into
flutter:masterfrom
fzyzcjy:patch-8
Closed

Remove workaround since #66006 is fixed#106913
fzyzcjy wants to merge 2 commits into
flutter:masterfrom
fzyzcjy:patch-8

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Jul 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

The comments say "@jiahaog Remove when #66006 is fixed." and that issue is already fixed

List which issues are fixed by this PR. You must list at least one issue.
#66006

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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, 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.
  • 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 f: integration_test The flutter/packages/integration_test plugin label Jul 1, 2022
@jiahaog

jiahaog commented Jul 1, 2022

Copy link
Copy Markdown
Member

Thank you! Can you take a look at the test failures? I think you need to remove

flutter/packages/integration_test/test/binding_test.dart

Lines 126 to 131 in 1add0d7

// TODO(jiahaog): Remove when https://github.com/flutter/flutter/issues/66006 is fixed.
testWidgets('root widgets are wrapped with a RepaintBoundary', (WidgetTester tester) async {
await tester.pumpWidget(const Placeholder());
expect(find.byType(RepaintBoundary), findsOneWidget);
});

@fzyzcjy

fzyzcjy commented Jul 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

You are right. I have removed them. (I forgot to check the CI just now...)

@fzyzcjy

fzyzcjy commented Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

@jiahaog Now tests seem to pass

@fzyzcjy

fzyzcjy commented Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

only remove code (no modified or added lines) to remove a feature or remove dead code.

So seems it is test-exempt

jiahaog
jiahaog previously approved these changes Jul 1, 2022
View reviewed changes
@jiahaog
jiahaog requested a review from dnfield July 1, 2022 08:21
@jiahaog

jiahaog commented Jul 1, 2022

Copy link
Copy Markdown
Member

I ran a global presubmit (tap/458352815) for this change. This will require a g3fix internally.

@jiahaog

jiahaog commented Jul 1, 2022

Copy link
Copy Markdown
Member

Actually I'm not really sure why removing the repaint boundary results in a significant resize of the screenshot goldens. @dnfield can you take a quick look at shortn/_z3wWrukdZc?

@fzyzcjy

fzyzcjy commented Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

What is tap/458352815 and shortn/_z3wWrukdZc...? Is it some internal links that I cannot view?

@jiahaog

jiahaog commented Jul 1, 2022

Copy link
Copy Markdown
Member

Oh don't worry about it, yes it's an internal system for running tests :)

@dnfield

dnfield commented Jul 1, 2022

Copy link
Copy Markdown
Contributor

From what I recall, this needed some changes internally to pump widgets - cl/392929256

@jiahaog

jiahaog commented Jul 4, 2022

Copy link
Copy Markdown
Member

Ah thanks, I didn't realise that the fix (PR #88609) for issue #66006 was actually reverted in PR #88889 so the issue shouldn't be closed, and I completely forgot all about the discussion we had previously regarding this.

Checking through internal tests failures again, it seems like this change causes some test to be especially flaky. This is not the same issue which caused it to be reverted though. I think the next step would be for us to investigate further to find out what is wrong with that test. We can track that on #66006, which I just reopened. Sorry about that!

@jiahaog
jiahaog dismissed their stale review July 4, 2022 04:32

Requires additional work to debug the flaky test internally as mentioned in the comment

@jiahaog jiahaog mentioned this pull request Jul 4, 2022
Closed
@fzyzcjy

fzyzcjy commented Aug 24, 2022

Copy link
Copy Markdown
Contributor Author

👀 Any updates here after two months? "workaround" does not sound very good so hoping it can be removed

@dnfield

dnfield commented Oct 27, 2022

Copy link
Copy Markdown
Contributor

Sorry, this should probably be closed. There is usage of this API internally in google that needs some reworking before this can be updated. Thank you for the effort!

@dnfield dnfield closed this Oct 27, 2022
@fzyzcjy

fzyzcjy commented Oct 27, 2022

Copy link
Copy Markdown
Contributor Author

You are welcome!

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

Reviewers

@jiahaog jiahaog jiahaog left review comments
@dnfield dnfield Awaiting requested review from dnfield

Assignees

No one assigned

Labels

f: integration_test The flutter/packages/integration_test plugin

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fzyzcjy @jiahaog @dnfield