Skip to content

Conversation

yaakovschectman
Copy link
Contributor

Adds Windows-desktop versions of the following tests (suffixed with _win_desktop):

  • flutter_gallery compile
  • flutter_gallery startup
  • complex_layout compile
  • complex_layout startup
  • flutter_view startup
  • platform_view startup

Also, renames hello_world_windows__compile to hello_world_win_desktop__compile for better clarity and consistency with file naming conventions.

As was necessary for these tests to run and pass, this PR also adds the functionality in perf_tests.dart to run startup tests on Windows.

@flutter/desktop is specified as the test owner team for each new test. All tests have the same configuration in .ci.yaml except for their target and task names. Please check the YAML file looks properly set up for the new targets.

Addresses issue #70027

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 d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. labels Aug 16, 2022
@yaakovschectman
Copy link
Contributor Author

Leaving as a draft for now to see which tests fail. Once they pass, I will mark ready for review.

@yaakovschectman yaakovschectman marked this pull request as ready for review August 16, 2022 16:28
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm for:

  • the runner checkin for complex_layout
  • the runner checkin for flutter_view
  • the runner checkin for platform_view
  • The task rename for the existing windows build test
  • TESTOWNERS changes

Wait for @CaseyHillers feedback on whether the TODO issue link is required for new tests, and if so add a link. Question for @CaseyHillers -- if that is the case, is the existing desktop issue linked in the PR description sufficient or do we need a new link specific to the removal of the bringup tag?

Last thing - have you verified the platform_view app builds and runs successfully? If I recall correctly, that sample relies on the battery level plugin and I'm not sure whether or not it has a Windows implementation.

@yaakovschectman
Copy link
Contributor Author

have you verified the platform_view app builds and runs successfully?

It builds and launches in Windows, with a sort of "press the button to increment this counter" demo, which works. There is a "continue in Android view" button that does not seem to do anything on Windows, but looking at its main.dart, I do not see any use of the battery level plugin, nor in its dependencies.

@cbracken
Copy link
Member

Sorry my brain completely blipped and was thinking of the platform channel example (I think under the examples/api sample) which uses the battery level plugin.

The reason for the android reference is that we currently assume only iOS/Android in the code. This will need fixing.

I believe you'll also need to do some tweaking of the native side to:

  • add a separate Win32 view that includes some text that shows a count. https://github.com/flutter/flutter/blob/master/examples/platform_view/ios/Runner/PlatformViewController.m
  • register a method call handler that implements a switchView method.
    FlutterMethodChannel* channel =
    [FlutterMethodChannel methodChannelWithName:@"samples.flutter.io/platform_view"
    binaryMessenger:controller];
    [channel setMethodCallHandler:^(FlutterMethodCall* call, FlutterResult result) {
    if ([@"switchView" isEqualToString:call.method]) {
    _flutterResult = result;
    PlatformViewController* platformViewController =
    [controller.storyboard instantiateViewControllerWithIdentifier:@"PlatformView"];
    platformViewController.counter = ((NSNumber*)call.arguments).intValue;
    platformViewController.delegate = self;
    UINavigationController* navigationController =
    [[UINavigationController alloc] initWithRootViewController:platformViewController];
    navigationController.navigationBar.topItem.title = @"Platform View";
    [controller presentViewController:navigationController animated:NO completion:nil];
    } else {
    result(FlutterMethodNotImplemented);
    }
  • fix up the code above to use a switch so we don't assume iOS/Android.

It may be enough work that it's worth doing in a separate PR if you'd like to do it now.

@yaakovschectman
Copy link
Contributor Author

[ ] add a separate Win32 view that includes some text that shows a count.

It appears that we may not yet actually have Windows support for native platform views (see #31713 ), other than possibly starting from scratch directly with the win32 API, in which case, I am not aware if there is a way to add a new win32 view within the running Flutter app (which I believe is the analogous functionality to what this button does in Android and iOS), or how one would go about doing so, if there is a way.

Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ci yaml configs LGTM.

@cbracken
Copy link
Member

cbracken commented Aug 16, 2022

how one would go about doing so, if there is a way.

Both mobile tests involve a navigation change. This isn't a typical desktop UI idiom, though some instances of this exist such as NSPreferencePanes in the macOS System Preferences app. One possibility on desktop might be to pop up an alert/dialog, but it really depends on how our tests interact with this app.

Comment on lines +221 to +227
/dev/devicelab/bin/tasks/hello_world_win_desktop__compile.dart @schectman @flutter/desktop
/dev/devicelab/bin/tasks/flutter_gallery_win_desktop__compile.dart @schectman @flutter/desktop
/dev/devicelab/bin/tasks/flutter_gallery_win_desktop__start_up.dart @schectman @flutter/desktop
/dev/devicelab/bin/tasks/complex_layout_win_desktop__compile.dart @schectman @flutter/desktop
/dev/devicelab/bin/tasks/complex_layout_win_desktop__start_up.dart @schectman @flutter/desktop
/dev/devicelab/bin/tasks/flutter_view_win_desktop__start_up.dart @schectman @flutter/desktop
/dev/devicelab/bin/tasks/platform_view_win_desktop__start_up.dart @schectman @flutter/desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaakovschectman Could you update the owner to github username instead of the ldap? Flake bot relies on github username to auto assign bugs/PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: contributor-productivity Team-specific productivity, code health, technical debt. d: examples Sample code and demos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants