Use ShellTest::Config struct to create Shell #42332

dkwingsmt · 2023-05-26T00:26:57Z

This PR refactors ShellTest::CreateShell so that instead of taking a list of optional parameters, it takes a Config struct, almost all fields of which have default values. This makes calling this method cleaner, and makes it easier to add more parameters. A short version is also kept that takes settings and task_runners, since this version is used so often.

Currently ShellTest::CreateShell's full version has 2 mandatory parameters and 5 optional parameters. Many calls to this method have to list a long list of default parameters before going to the single one that needs overwriting, and adding new parameters to this call is even more tedious.

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 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 ///).
I signed the CLA.
All existing and new tests are passing.

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

dnfield

I like this idea, the config object could be cleaned up a bit.

If assigning defaults needs more than simple logic (like perhaps the default for task_runners), consider using a builder pattern.

dnfield · 2023-05-30T18:18:17Z

shell/common/shell_test.h

+    // Required.
+    const Settings& settings;
+    // Defaults to &GetTaskRunnersForFixture().
+    const TaskRunners* task_runners;


Please don't use a raw pointer here.

Just keep this as a value - avoids one more semantic change in the way these tests work, and avoids ambiguity around whether this can be nullptr and what that means.

Also in general here: instead of specifying defaults in a comment, initialize the values here with a default.

zanderso · 2023-06-08T20:09:39Z

From PR review triage: Converting to a draft.

flutter-dashboard · 2023-06-08T20:15:31Z

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.

dnfield · 2023-06-09T22:11:12Z

shell/common/shell_test.h

+    // Required.
+    const Settings& settings;
+    // Defaults to GetTaskRunnersForFixture().
+    std::optional<TaskRunners> task_runners = {};


This is where I think the builder could be helpful - you don't want the public interface here to actually be optional.

But if you don't want to use a builder I think we should make the std::optional be on the constructor for this struct and make the member non-optional - because otherwise callers will always have to check (and get linted against not checking now) whether the optional has a value when it really always does.

Not that I don't want to use a builder, but here's the idea: I can create a TaskRunnersBuilder, which has two ctors:

// implicit conversion, so that existing tests work without modification TaskRunnersBuilder(TaskRunner); // Defaults to GetTaskRunnersForFixture() TaskRunnersBuilder();

But I think it's really just reinventing the wheel for std::optional because it's not semantically obvious what the default behavior is (and the doc is now hidden even deeper).

And what's worse, because this field is a value, not a pointer of a function, I can't really specify an "empty" value if not for the default ctor.

As for

because otherwise callers will always have to check (and get linted against not checking now) whether the optional has a value when it really always does

I don't think this is a problem, because this struct really is only used by ShellTest::CreateShell.

Another option is to mark task_runners as mandatory so that you always explicit write GetTaskRunnersForFixture() for every call.

dkwingsmt · 2023-06-09T22:43:13Z

Also I created a builder for PlatformView. Let me know what you think!

dnfield · 2023-06-11T03:38:15Z

shell/common/shell_test_platform_view.cc

+ShellTestPlatformViewBuilder::ShellTestPlatformViewBuilder(Config config)
+    : config_(config) {}
+
+std::unique_ptr<PlatformView> ShellTestPlatformViewBuilder::operator()(


nit: avoid defining a custom operator for this, just make a Build method.

The custom operator is to make this class assignable to the current std::function parameter. We can turn this into a Build method instead, but then the several cases that currently define a custom closure will have to define a new class that takes a closure. Should I do it?

Shell::CreateCallback<PlatformView> platform_view_create_callback = [task_runners, main_context](flutter::Shell& shell) { auto result = std::make_unique<TestPlatformView>(shell, task_runners); ON_CALL(*result, CreateRenderingSurface()) .WillByDefault(::testing::Invoke([main_context] { auto surface = std::make_unique<MockSurface>(); ON_CALL(*surface, GetContext()) .WillByDefault(Return(main_context.get())); ON_CALL(*surface, IsValid()).WillByDefault(Return(true)); ON_CALL(*surface, MakeRenderContextCurrent()) .WillByDefault(::testing::Invoke([] { return std::make_unique<GLContextDefaultResult>(true); })); return surface; })); return result; }; auto shell = CreateShell({ .settings = settings, .task_runners = task_runners, .platform_view_create_callback = platform_view_create_callback, });

engine/shell/common/shell_unittests.cc

Line 1608 in 6699bbb

Shell::CreateCallback<PlatformView> platform_view_create_callback =

That's fine then.

dnfield

LGTM with nit

auto-submit · 2023-06-11T08:29:22Z

auto label is removed for flutter/engine, pr: 42332, due to - The status or check suite Linux Host clang-tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

…e#42332)

…128669) flutter/engine@6e2c71a...5da44b9 2023-06-11 [email protected] Use ShellTest::Config struct to create Shell (flutter/engine#42332) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter-dashboard bot added platform-android platform-fuchsia platform-ios labels May 26, 2023

dkwingsmt force-pushed the shell_test_config branch from c747b46 to b1ea24a Compare May 26, 2023 07:24

dkwingsmt marked this pull request as ready for review May 26, 2023 07:49

dkwingsmt force-pushed the shell_test_config branch from 19bcecb to 0a9a584 Compare May 26, 2023 21:09

dkwingsmt requested a review from jonahwilliams May 26, 2023 22:10

dnfield suggested changes May 30, 2023

View reviewed changes

zanderso marked this pull request as draft June 8, 2023 20:09

Use optional for task runners

d96abce

dkwingsmt force-pushed the shell_test_config branch from 9a9e92a to d96abce Compare June 9, 2023 21:43

github-actions bot removed platform-fuchsia platform-android labels Jun 9, 2023

dnfield reviewed Jun 9, 2023

View reviewed changes

ShellTestPlatformViewBuilder

6699bbb

dnfield reviewed Jun 11, 2023

View reviewed changes

dnfield approved these changes Jun 11, 2023

View reviewed changes

dkwingsmt added 2 commits June 10, 2023 23:18

Fix crash

110d62a

Fix lint and add comment

a84de69

dkwingsmt marked this pull request as ready for review June 11, 2023 07:58

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 11, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 11, 2023

Lint fix

2ff1f92

dkwingsmt merged commit 5da44b9 into flutter:main Jun 11, 2023

engine-flutter-autoroll mentioned this pull request Jun 11, 2023

Roll Flutter Engine from 6e2c71adccad to 5da44b9aafdd (1 revision) flutter/flutter#128669

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 11, 2023

5da44b9aa Use ShellTest::Config struct to create Shell (flutter/engin…

8311bcd

…e#42332)

HansMuller mentioned this pull request Jun 16, 2023

Flutter Framework Team: Notable Commits flutter/flutter#121415

Closed

Use ShellTest::Config struct to create Shell #42332

Use ShellTest::Config struct to create Shell #42332

Uh oh!

Conversation

dkwingsmt commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield May 30, 2023

Choose a reason for hiding this comment

Uh oh!

dnfield May 30, 2023

Choose a reason for hiding this comment

Uh oh!

zanderso commented Jun 8, 2023

Uh oh!

flutter-dashboard bot commented Jun 8, 2023

Uh oh!

dnfield Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Jun 9, 2023

Uh oh!

dnfield Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jun 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

dnfield Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jun 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dkwingsmt commented May 26, 2023 •

edited

Loading

dkwingsmt Jun 9, 2023 •

edited

Loading

dkwingsmt Jun 11, 2023 •

edited

Loading