Fix abstract_method_smoke_test flakiness #42454

mklim · 2019-10-10T17:56:26Z

Description

The flake appears to be coming from the Google Maps plugin. This test
just needs a platform view plugin without the interface method
implemented in general, not Maps specifically. Update it here to avoid
the issue in Maps until that's fixed.

Create a very simple platform view implementation that exercises this
potential bug in order to create a minimal test case with less risk of
causing second order issues like depending on any kind of full package
would cause.

Related Issues

Fixes #42349
See also #42451

Tests

Updated abstract_method_smoke_test

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

fluttergithubbot · 2019-10-10T17:56:28Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

The flake appears to be coming from the Google Maps plugin. This test just needs a platform view plugin without the interface method implemented in general, not Maps specifically. Update it here to avoid the issue in Maps until that's fixed.

dnfield

LGTM!

mklim · 2019-10-10T18:39:57Z

Hmm. Is there a way to tell the analyzer to ignore an "invalid" dependency? I'm deliberately using an outdated version of webview_flutter because that's the version of the package that would cause this test to go red without the fix. I know normally you can use ignore: for analyzer lints, but I'm having trouble finding the right name for this one.

ETA: I "fixed" this by temporarily locally patching update_packages.dart to have a pinned manual dependency so that it created a "valid" CHECKSUM and then committing that, but I don't think that's the right way to go long term. The next time this needs to be --force-upgraded for some other reason I think it will be overwritten. But I don't want to actually add this package into update_packages.dart as a globally pinned dependency because we really shouldn't be pinning the dependency to this universally, it's an old version. We just want to use it for this one integration test. It would be better if we could exclude this check for this line or directory somehow. WDYT @dnfield ?

This was done by locally patching `update_packages.dart` to have a new manually pinned dependency on webview_flutter: 0.3.9+1.

codecov · 2019-10-10T19:19:26Z

Codecov Report

Merging #42454 into master will decrease coverage by 0.69%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #42454     +/-   ##
=========================================
- Coverage   60.37%   59.68%   -0.7%     
=========================================
  Files         194      194             
  Lines       18849    18849             
=========================================
- Hits        11381    11250    -131     
- Misses       7468     7599    +131

Flag	Coverage Δ
#flutter_tool	`59.68% <ø> (-0.7%)`	⬇️

Impacted Files	Coverage Δ
...tter_tools/lib/src/build_system/targets/linux.dart	`38.29% <0%> (-46.81%)`	⬇️
packages/flutter_tools/lib/src/web/web_device.dart	`7.22% <0%> (-40.97%)`	⬇️
packages/flutter_tools/lib/src/ios/mac.dart	`31.66% <0%> (-22.58%)`	⬇️
packages/flutter_tools/lib/src/base/build.dart	`58.67% <0%> (-15.71%)`	⬇️
...ages/flutter_tools/lib/src/linux/linux_doctor.dart	`75% <0%> (-15%)`	⬇️
packages/flutter_tools/lib/src/asset.dart	`82.62% <0%> (-3.39%)`	⬇️
...ckages/flutter_tools/lib/src/reporting/events.dart	`92.95% <0%> (-2.82%)`	⬇️
packages/flutter_tools/lib/src/device.dart	`63.25% <0%> (-2.41%)`	⬇️
packages/flutter_tools/lib/src/context_runner.dart	`67.24% <0%> (-1.73%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`47.54% <0%> (-1.64%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a63ef7...8766433. Read the comment docs.

mklim · 2019-10-10T20:14:09Z

🤦‍♂️ I forgot that there was a webview bug in this version that was fixed in later versions, https://firebase.corp.google.com/u/0/project/flutter-infra/testlab/histories/bh.1b29258e8f97b976/matrices/8877227498452021020/executions/bs.32007b1eae22dd0. I'm going to just rewrite this to use a custom platform view that does nothing. Sorry about the back and forth.

Before, the test was depending on a specific package version that demonstrated the potential bug. However the flutter analyzer demands updated package versions and we'd need to work around it here to keep the version in sync. In addition to that the old package version itself had a different bug in addition to the previous bug this patch is avoiding from the _other_ package that previously caused the test to fail in CI. The newer version of the package without the bug also didn't exercise the right code path to make this test meaningful. Create a very simple platform view implementation that exercises this potential bug in order to create a minimal test case with less risk of causing second order issues like depending on any kind of full package would cause.

mklim · 2019-10-10T21:38:53Z

OK, updated the test to use a simple custom platform view instead of relying on a plugin at all. @blasten and @dnfield please take a look at the latest patches when you have a minute, the PR has changed a bit since last LGTM.

blasten · 2019-10-10T23:23:54Z

...moke_test/android/app/src/main/kotlin/com/example/abstract_method_smoke_test/MainActivity.kt

+            view.setBackgroundColor(Color.CYAN)
+        }
+
+        override fun dispose() {}


not sure if the empty dispose() is needed, but just in case.

Hard to tell in Kotlin, but the original class is a Java interface so it's still needed here.

blasten · 2019-10-10T23:24:43Z

LGTM. Thanks for the effort to track this down :)

The flake appears to be coming from the Google Maps plugin. This test just needs a platform view plugin without the interface method implemented in general, not Maps specifically. Update it here to avoid the issue in Maps until that's fixed. Create a very simple platform view implementation that exercises this potential bug in order to create a minimal test case with less risk of causing second order issues like depending on any kind of full package would cause.

mklim added the a: tests "flutter test", flutter_test, or one of our tests label Oct 10, 2019

mklim requested review from blasten and dnfield October 10, 2019 17:56

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Oct 10, 2019

googlebot added the cla: yes label Oct 10, 2019

Fix abstract_method_smoke_test flakiness

e953357

The flake appears to be coming from the Google Maps plugin. This test just needs a platform view plugin without the interface method implemented in general, not Maps specifically. Update it here to avoid the issue in Maps until that's fixed.

dnfield approved these changes Oct 10, 2019

View reviewed changes

blasten approved these changes Oct 10, 2019

View reviewed changes

mklim added the waiting for tree to go green label Oct 10, 2019

mklim removed the waiting for tree to go green label Oct 10, 2019

Generate a new valid CHECKSUM.

da0e81c

This was done by locally patching `update_packages.dart` to have a new manually pinned dependency on webview_flutter: 0.3.9+1.

mklim added the work in progress; do not review label Oct 10, 2019

mklim removed the work in progress; do not review label Oct 10, 2019

Inline trivial factory

8766433

blasten reviewed Oct 10, 2019

View reviewed changes

dnfield approved these changes Oct 10, 2019

View reviewed changes

mklim added the waiting for tree to go green label Oct 10, 2019

mklim merged commit 6d18d31 into flutter:master Oct 11, 2019

mklim deleted the real_deflake branch October 11, 2019 01:23

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix abstract_method_smoke_test flakiness #42454

Fix abstract_method_smoke_test flakiness #42454

Uh oh!

mklim commented Oct 10, 2019 •

edited

Loading

Uh oh!

fluttergithubbot commented Oct 10, 2019

Uh oh!

dnfield left a comment

Uh oh!

mklim commented Oct 10, 2019 •

edited

Loading

Uh oh!

codecov bot commented Oct 10, 2019 •

edited

Loading

Uh oh!

mklim commented Oct 10, 2019

Uh oh!

mklim commented Oct 10, 2019

Uh oh!

blasten Oct 10, 2019

Uh oh!

mklim Oct 10, 2019

Uh oh!

blasten commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix abstract_method_smoke_test flakiness #42454

Fix abstract_method_smoke_test flakiness #42454

Uh oh!

Conversation

mklim commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Oct 10, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

mklim commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mklim commented Oct 10, 2019

Uh oh!

mklim commented Oct 10, 2019

Uh oh!

blasten Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

blasten commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mklim commented Oct 10, 2019 •

edited

Loading

mklim commented Oct 10, 2019 •

edited

Loading

codecov bot commented Oct 10, 2019 •

edited

Loading