Added unit tests to the engine. #20216

gaaclarke · 2020-08-04T00:40:05Z

Description

Added tests to the Engine, previously there were none. The tests that were added were for the benefit of #20193

In order to make messages testable without incurring a performance cost I had to factor out one method to be templated so that we can delegate RuntimeController methods elsewhere.

Related Issues

#20193

Tests

EngineTest

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 C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

gaaclarke · 2020-08-04T01:01:31Z

shell/common/engine.h

+  ///             `DispatchPlatformMessage` unless you are writing tests.
+  ///
+  template <typename RuntimeControllerProxy>
+  void DispatchPlatformMessage(fml::RefPtr<PlatformMessage> message,


This might not scale well in the future, we'll see. It was this or make RuntimeController not final and give it virtual methods, which would have runtime costs. RuntimeController is very complicated to use in tests since it requires a dart snapshot and everything that goes along with that.

Also worth noting, I tried to make this method private but that doesn't scale well since googletest compiles each test as its own class that has to be friended. I can still do that if you want. It's not breaking any encapsulation having it public, it is a bit cleaner.

chinmaygarde

This OOLs a bunch of code and I agree with your hunch that this won't scale well. If the behavior of a the component is hard to test without templating, visibility or const correctness hacks, it usually means that that it has more than a single responsibility. That seems to be the case here.

Instead, I suggest isolating the message dispatcher into its own component (something like PlatformMessageDispatcher) and testing that separately from the engine. Then engine can then use an instance of this tested component. The implementation of this new object would just be a proc lookup in a map keyed by the channel name which is pretty much what is happening here. The test assertions can check the correctness of the entries in this map. For this, you don't need any of the Dart VM bits at all.

gaaclarke · 2020-08-04T17:46:39Z

Thanks @chinmaygarde That approach has scaling issues as well. Right now engine.cc isn't tested and if every time we want to test it we have to split out the functionality to a separate class, that's not good either. Also, introducing PlatformMessageDispatcher or anything else to test in isolation may create an extra level of indirection (either through function pointers or virtual functions) which would have the performance cost we were trying to avoid by keeping RuntimeController's methods non-virtual. Let's chat about our options when you have a chance.

flar · 2020-08-04T18:21:52Z

Trying to avoid a virtual function call per platform message dispatch sounds like it might be premature optimization to me. Are there statistics that show that these messages come in at high enough volume to measure these nanoseconds?

gaaclarke · 2020-08-04T18:39:53Z

@flar It's an optimization, I wouldn't call it premature though. This is engine code in the place where messages are getting dispatched which can get some heavy usage. Virtual functions add just a couple of cycles to calls but more importantly they block optimizations like inlining. In isolation they don't matter much, but in aggregate across something like the engine it can make a big difference.

I'm fine making the methods virtual. I didn't think this was that bad of a tradeoff to get testability with no performance cost though. As we add more tests I'm skeptical this will last though and we'll probably have to migrate to virtual methods anyway.

gaaclarke · 2020-08-04T21:55:47Z

shell/common/engine_unittests.cc

+class MockRuntimeDelegate : public RuntimeDelegate {
+ public:
+  MOCK_METHOD(std::string, DefaultRouteName, (), (override));
+  MOCK_METHOD(void, ScheduleFrame, (bool), (override));
+  MOCK_METHOD(void, Render, (std::unique_ptr<flutter::LayerTree>), (override));
+  MOCK_METHOD(void,
+              UpdateSemantics,
+              (SemanticsNodeUpdates, CustomAccessibilityActionUpdates),
+              (override));
+  MOCK_METHOD(void,
+              HandlePlatformMessage,
+              (fml::RefPtr<PlatformMessage>),
+              (override));
+  MOCK_METHOD(FontCollection&, GetFontCollection, (), (override));
+  MOCK_METHOD(void,
+              UpdateIsolateDescription,
+              (const std::string, int64_t),
+              (override));
+  MOCK_METHOD(void, SetNeedsReportTimings, (bool), (override));
+
+  MOCK_METHOD(std::unique_ptr<std::vector<std::string>>,
+              ComputePlatformResolvedLocale,
+              (const std::vector<std::string>&),
+              (override));
+};


I could get rid of the need for this if we were willing to change the RuntimeControllers instance variable for its RuntimeDelegate from a reference to a pointer. I think thats a desirable change but I suspected it might be controversial.

gaaclarke · 2020-08-04T21:56:26Z

@chinmaygarde @flar I moved to virtual methods in the RuntimeController =)

gaaclarke · 2020-08-06T16:39:38Z

One sec, I'm still battling a bug in googletest on windows. I think if i used the deprecated methods MOCK_METHOD0 versus MOCK_METHOD it should work.

flar · 2020-08-07T18:29:03Z

My "food for thought" comment above was the extent of my review, so consider the matter resolved if you took it under consideration and came up with an acceptable solution...

gaaclarke · 2020-08-07T22:55:31Z

Landing before last CI test passes. This last test previously passed, the only change since then should be covered in other tests. When the test first ran it had an infra error. When it was rerun it got queued for over 2 hours. Attempts to launch it manually with led failed with "permission denied":

$ led get-builder 'luci.flutter.try:Mac Host Engine' 
E0807 15:52:40.850576   74654 fleetspeak.go:79] Error flushing fleetspeak data: (downstream) write of 13191 bytes (compressed payload) + metadata: Dial("/var/run/fleetspeak/clp_exporter.sock", "CL:324807037"): dial unix /var/run/fleetspeak/clp_exporter.sock: connect: permission denied
E0807 15:53:40.732003   74654 fleetspeak.go:79] Error flushing fleetspeak data: previous message still pending send: (downstream) write of 13191 bytes (compressed payload) + metadata: Dial("/var/run/fleetspeak/clp_exporter.sock", "CL:324807037"): dial unix /var/run/fleetspeak/clp_exporter.sock: connect: permission denied

googlebot added the cla: yes label Aug 4, 2020

gaaclarke force-pushed the engine-add-tests branch from 8777c4b to 11f3f21 Compare August 4, 2020 00:46

gaaclarke marked this pull request as ready for review August 4, 2020 00:48

gaaclarke requested review from jason-simmons, mehmetf and flar August 4, 2020 00:48

auto-assign bot requested a review from GaryQian August 4, 2020 00:48

gaaclarke removed the request for review from GaryQian August 4, 2020 00:54

gaaclarke force-pushed the engine-add-tests branch from 11f3f21 to 6a0d3a4 Compare August 4, 2020 00:58

gaaclarke commented Aug 4, 2020

View reviewed changes

chinmaygarde requested changes Aug 4, 2020

View reviewed changes

gaaclarke commented Aug 4, 2020

View reviewed changes

gaaclarke requested a review from chinmaygarde August 4, 2020 21:55

gaaclarke force-pushed the engine-add-tests branch from 75dddd2 to 45dcf58 Compare August 5, 2020 17:03

chinmaygarde approved these changes Aug 6, 2020

View reviewed changes

gaaclarke added 10 commits August 6, 2020 17:20

Added unit tests to the engine.

9dc7f75

added missing license check

1cff1c5

switched to mocking out the runtimecontroller

7932aa8

added space

b39f6a0

removed unused method

cf87907

moved the channel names back into .cc

d352848

ran formatter

3a586fc

tried to fix windows builds

f5d6a2b

formatter

6e6bffd

switched to deprecated mock declarations to support windows bug

31be985

gaaclarke force-pushed the engine-add-tests branch from 775b93a to 31be985 Compare August 7, 2020 00:20

gaaclarke merged commit 2ea8396 into flutter:master Aug 7, 2020

engine-flutter-autoroll mentioned this pull request Aug 8, 2020

Roll Engine from 1e50d87dc85f to 409a5e596349 (25 revisions) flutter/flutter#63261

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 8, 2020

2ea8396 Added unit tests to the engine. (flutter/engine#20216)

4c21545

engine-flutter-autoroll mentioned this pull request Aug 8, 2020

Roll Engine from 1e50d87dc85f to e50219fdc5cc (26 revisions) flutter/flutter#63262

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 8, 2020

2ea8396 Added unit tests to the engine. (flutter/engine#20216)

33a4218

engine-flutter-autoroll mentioned this pull request Aug 8, 2020

Roll Engine from 1e50d87dc85f to d7c98913ed9c (27 revisions) flutter/flutter#63267

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 8, 2020

2ea8396 Added unit tests to the engine. (flutter/engine#20216)

cf00707

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added unit tests to the engine. #20216

Added unit tests to the engine. #20216

gaaclarke commented Aug 4, 2020

gaaclarke Aug 4, 2020 •

edited

gaaclarke Aug 4, 2020

chinmaygarde left a comment

gaaclarke commented Aug 4, 2020

flar commented Aug 4, 2020

gaaclarke commented Aug 4, 2020

gaaclarke Aug 4, 2020

gaaclarke commented Aug 4, 2020

gaaclarke commented Aug 6, 2020

flar commented Aug 7, 2020

gaaclarke commented Aug 7, 2020

Added unit tests to the engine. #20216

Added unit tests to the engine. #20216

Conversation

gaaclarke commented Aug 4, 2020

Description

Related Issues

Tests

Checklist

Breaking Change

gaaclarke Aug 4, 2020 • edited

Choose a reason for hiding this comment

gaaclarke Aug 4, 2020

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

gaaclarke commented Aug 4, 2020

flar commented Aug 4, 2020

gaaclarke commented Aug 4, 2020

gaaclarke Aug 4, 2020

Choose a reason for hiding this comment

gaaclarke commented Aug 4, 2020

gaaclarke commented Aug 6, 2020

flar commented Aug 7, 2020

gaaclarke commented Aug 7, 2020

gaaclarke Aug 4, 2020 •

edited