engine icon indicating copy to clipboard operation
engine copied to clipboard

Published 20 hours ago • verified publisher icon flutter

Re-landing Robolectric 4.8.1

Open utzcoz opened this issue 2 years ago • 12 comments

Re-landing https://github.com/flutter/engine/pull/33024

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] 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 ///).
  • [x] I signed the CLA.
  • [x] All existing and new tests are passing.

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

utzcoz avatar Jun 24 '22 14:06 utzcoz

@utzcoz What's the status of this PR? Were you able to determine the cause of the failures?

Hixie avatar Sep 27 '22 23:09 Hixie

@utzcoz What's the status of this PR? Were you able to determine the cause of the failures?

@Hixie Sorry, I didn't have enough time to resolve test failures when updating Robolectric and latest's Flutter Engine building on my machine. I will try it again this weekend, and I might bump Robolectric to 4.9 directly if it can be released this week.

utzcoz avatar Sep 28 '22 02:09 utzcoz

java.lang.IllegalArgumentException: Window type mismatch. Window Context's window type is 2037, while LayoutParams' type is set to 2030. Please create another Window Context via createWindowContext(getDisplay(), 2030, null) to add window with type:2030

The presentation.show() in VirtualDisplayController throws an exception when running tests. It is the direct reason that test failed.

utzcoz avatar Sep 30 '22 16:09 utzcoz

Looks like https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/app/Presentation.java;l=228;drc=185824b521e1e883ddc947e17257e1363cb6b25f;bpv=1;bpt=1 might set different window type than SingleViewPresentation.java sets in LayoutParams in higher version. I will check whether Robolectric supports it correctly.

utzcoz avatar Sep 30 '22 16:09 utzcoz

Looks like https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/app/Presentation.java;l=228;drc=185824b521e1e883ddc947e17257e1363cb6b25f;bpv=1;bpt=1 might set different window type than SingleViewPresentation.java sets in LayoutParams in higher version. I will check whether Robolectric supports it correctly.

I think that Robolectric doesn't support it correctly on higher SDK version, and I file a new issue at https://github.com/robolectric/robolectric/issues/7663.

utzcoz avatar Oct 01 '22 03:10 utzcoz

Hi @Hixie , I used two custom shadows to fix test failures with Robolectric 4.8.1 to avoid changing Flutter origin logic. But I think it's better some folks can help to revisit affected tests based on proper logic/assumption of Surface. I am not familiar with Flutter internal, so I can't modify these logic quickly to meet project's requirement.

utzcoz avatar Oct 01 '22 05:10 utzcoz

Hi @zanderso , could you help to review this PR? Thanks.

utzcoz avatar Oct 05 '22 05:10 utzcoz

cc @stuartmorgan for routing on https://github.com/flutter/engine/pull/34272#issuecomment-1264253576. Since the tests are passing, LGTM.

chinmaygarde avatar Oct 06 '22 20:10 chinmaygarde

I don't quite follow the explanation (perhaps because I don't have a lot of experience with Robolectric). @utzcoz Can you elaborate on what the problem in the test is exactly?

stuartmorgan avatar Oct 07 '22 12:10 stuartmorgan

I don't quite follow the explanation (perhaps because I don't have a lot of experience with Robolectric). @utzcoz Can you elaborate on what the problem in the test is exactly?

@stuartmorgan From Robolectric 4.8.x, Robolectric implements a simple faker implmentation for Surface#lockHardwareCanvas, and it will pass without error. But looks like disposeNullAndroidView test assumes Surface#lockHardwareCanvas failed to pass test. This PR adds a new ShadowSurface to ensure Surface#lockHardwareCanvasthrows Exception as tests expected. But from 4.9, Robolectric makes Surface#lockHardwareCanvas more realistic, I think we can remove this ShadowReelaseSurface when updating Robolectric to 4.9 for Flutter/engine. cc @chinmaygarde .

utzcoz avatar Oct 07 '22 12:10 utzcoz

Sorry, I tried to Robolectric 4.9, looks like default ShadowSurface didn't work as expected. I will investigate whether it when I bump Robolectric to 4.9 for flutter/engine.

utzcoz avatar Oct 07 '22 12:10 utzcoz

Hello, are there any updates for this PR? I hope it can be merged, and I can start to work for updating Robolectric to 4.9.

utzcoz avatar Oct 10 '22 13:10 utzcoz