Test integration test apps' runner files against current template app #118646

yaakovschectman · 2023-01-17T22:04:58Z

Add a script and CI test to ensure that the files present in the Windows subdirectories for integration tests match those of the app template in order to ensure that any changes made to the template are reflected in the integration tests.

Part of #118643

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.

yaakovschectman · 2023-01-17T22:05:47Z

dev/devicelab/bin/tasks/windows_app_template_test.dart

+import 'package:path/path.dart';
+
+const int _kLicenseLines = 4;
+const List<String> _kFilesToMatch = <String>[


The files that are expected to always be identical between the template and the integration tests. Some files, e.g. flutter_window.cpp are expected to be different, as they may contain test-specific code, such as registering custom method handlers.

Thanks for adding this. Question for @loic-sharma and @yaakovschectman. Any preferences on an allow-list (as is currently implemented) vs ignore-list (everything under the directory except for listed files) approach for this?

An ignore-list has the advantage that if someone ever adds more files, they won't need to remember to add them here.

An ignore list seems like a great idea 👍

yaakovschectman · 2023-01-17T22:05:59Z

dev/integration_tests/windows_startup_test/windows/runner/main.cpp

@@ -5,7 +5,6 @@
 #include <flutter/dart_project.h>
 #include <flutter/flutter_view_controller.h>
 #include <windows.h>
-#include <windef.h>


Seemingly unneeded deviation from the template file.

loic-sharma · 2023-01-18T17:52:56Z

dev/devicelab/bin/tasks/windows_app_template_test.dart

+Future<void> main() async {
+  await task(() async {
+    final String integrationTestsPath = '${flutterDirectory.path}$_kIntegrationTestsRelativePath';
+    final String templatePath = '${flutterDirectory.path}$_kTemplateRelativePath';


Consider using path.join from package:path.

loic-sharma · 2023-01-18T18:00:08Z

dev/devicelab/bin/tasks/windows_app_template_test.dart

+}
+
+Future<String> _fileContents(String path, {int linesToSkip = 0}) async {
+  return (await File(path).readAsLines()).sublist(linesToSkip).join('\n');


I feel uneasy with completely ignoring 4 lines. Should we verify the app has the expected license?

Do we not already do so with an existing test for all files? I thought one of our tests ensures any files with certain extensions that are touched by a commit contain a license, but if I am mistaken I can add such a check.

Yup there is a license check, however, my concern is with us missing unexpected content. Imagine that our license is shortened to 3 lines and that an erroneous 4th line is added to an integration test file. The test wouldn't catch this scenario.

I'd hope we do, but I'd treat those tests as orthogonal, since (a) it simplifies the code and (b) the licences are a legitimate part of the file for which we don't want to see diffs.

@cbracken There would need to be a diff wrt the licenses because the template files lack licenses. I can check separately that the app file contents begin with the expected license and compare the remainder of the file to the template. Do you know if/where we have a file that contains the license prepended to the template files?

dev/devicelab/bin/tasks/windows_app_template_test.dart

loic-sharma · 2023-01-18T18:19:40Z

.ci.yaml

@@ -4915,3 +4915,17 @@ targets:
      validation: docs_deploy
      validation_name: Docs_deploy
      firebase_project: docs-flutter-dev
+
+  - name: Windows windows_app_template_test
+    recipe: devicelab/devicelab_drone


Does this need to be a devicelab test? These are "expensive" tests that run on limited physical devices. Can we run this test on whatever runs our unit tests?

Would that be flutter/flutter_drone? I believe that and devicelab run integration tests currently.

How long does this take? If it's <=1 minute, we can add it to https://github.com/flutter/flutter/blob/master/dev/bots/analyze.dart

Would that be flutter/flutter_drone? I believe that and devicelab run integration tests currently.

The issue is not so much concurrency, but the fact that we have a limited number of physical devicelab bots, and scheduling an additional build per commit will slightly increase queue time for all other builds needing that bot.

I will try adding it to analyze.dart then. It is a quick task.

@christopherfujino Do you know the file system set up that the analyze script is run on? I am trying to add this test to analyze as you suggested, and the requested file is not found, though it is when I run locally. Does the logic of getting the file paths in https://github.com/flutter/flutter/blob/4564aa854c25a9b395a8f324f8f506c72ad6b6e3/dev/bots/analyze.dart#L1890-L1903 seem mistaken to you?

See https://github.com/flutter/flutter/pull/118646/files#r1080535963

cbracken

Thanks for adding this. Overall looks great.

cbracken · 2023-01-18T18:30:19Z

dev/devicelab/bin/tasks/windows_app_template_test.dart

+import 'package:path/path.dart';
+
+const int _kLicenseLines = 4;
+const List<String> _kFilesToMatch = <String>[


Thanks for adding this. Question for @loic-sharma and @yaakovschectman. Any preferences on an allow-list (as is currently implemented) vs ignore-list (everything under the directory except for listed files) approach for this?

An ignore-list has the advantage that if someone ever adds more files, they won't need to remember to add them here.

christopherfujino · 2023-01-18T20:43:22Z

dev/bots/analyze.dart

+        String templateFile = _fileContents(templateFilePath);
+        String appFilePath = path.join(runnerPath, fileName);
+        if (fileName.endsWith('.tmpl')) {
+          appFilePath = appFilePath.substring(0, appFilePath.length - 4); // Remove '.tmpl' from app file path


You should be removing 5 chars here (forgot the .)

dev/bots/analyze.dart

loic-sharma · 2023-01-19T18:33:32Z

dev/bots/analyze.dart

+}
+
+Future<void> verifyIntegrationTestTemplateFiles(String flutterRoot) async {
+    final List<String> errors = <String>[];


Indentation seems off here

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

loic-sharma

Looks great! 🏅

…mplate app (flutter/flutter#118646)

…late app (flutter/flutter#118646)

* 373523184 Cleanup old Dart SDK layout compatibility (flutter/flutter#118819) * 4d250302a Add leak_tracker as dev_dependency. (flutter/flutter#118952) * e3c51a2f2 Add Windows unit tests to plugin template (flutter/flutter#118638) * d20dd9e4b Roll Flutter Engine from 7d3233d26d09 to 71ee5f19bc16 (15 revisions) (flutter/flutter#119081) * 5dabe102a Fix path name to discover debug apk on add2app builds (flutter/flutter#117999) * 50ed8a34b Enable `unnecessary_null_comparison` check (flutter/flutter#118849) * 455e6aca5 Test integration test apps' runner files against current template app (flutter/flutter#118646) * a788e1b31 Roll Flutter Engine from 71ee5f19bc16 to 59ea78bfabda (2 revisions) (flutter/flutter#119087) * c35370cf0 Roll Flutter Engine from 59ea78bfabda to 2499a5d9fca7 (2 revisions) (flutter/flutter#119089) * 2f0dd5673 Refactor highlight handling in FocusManager (flutter/flutter#119075) * 2759f3f0b Roll Flutter Engine from 2499a5d9fca7 to d98926c32ee7 (2 revisions) (flutter/flutter#119090) * 760fb2115 Roll Flutter Engine from d98926c32ee7 to bec40654a5d7 (2 revisions) (flutter/flutter#119093) * bbca694ef Roll Flutter Engine from bec40654a5d7 to 5405f2c26e85 (2 revisions) (flutter/flutter#119095) * 6414c3604 f1464b49c Manually roll ANGLE, vulkan-deps, SwiftShader (flutter/engine#38650) (flutter/flutter#119097) * 426cdd90c 55bb8deaf [Impeller] Linear sample atlas glyphs when the CTM isn't translation/scale only (flutter/engine#39112) (flutter/flutter#119098) * 83c3a61e3 Only emit image painting events in debug & profile modes. (flutter/flutter#118872) * b113df2dc bffb98352 Roll Skia from b72fececbdcc to 8ffd5c20d634 (3 revisions) (flutter/engine#39114) (flutter/flutter#119099) * 351466aea Add Decoding Flutter videos to API docs (flutter/flutter#116454) * 318f8758b Pass through magnifierConfiguration (flutter/flutter#118270) * eced23eab d39ab638b Roll Fuchsia Mac SDK from MUvFS0baOnigVUIND... to _H53AyDxR9Pm2TbwN... (flutter/engine#39122) (flutter/flutter#119126) * 29ab437e2 Add Material 3 `CheckboxListTile` example and update existing examples (flutter/flutter#118792) * a815ee634 8efc7183b Roll Skia from 8ffd5c20d634 to da5034f9d117 (4 revisions) (flutter/engine#39123) (flutter/flutter#119129)

* 3735231 Cleanup old Dart SDK layout compatibility (flutter/flutter#118819) * 4d25030 Add leak_tracker as dev_dependency. (flutter/flutter#118952) * e3c51a2 Add Windows unit tests to plugin template (flutter/flutter#118638) * d20dd9e Roll Flutter Engine from 7d3233d26d09 to 71ee5f19bc16 (15 revisions) (flutter/flutter#119081) * 5dabe10 Fix path name to discover debug apk on add2app builds (flutter/flutter#117999) * 50ed8a3 Enable `unnecessary_null_comparison` check (flutter/flutter#118849) * 455e6ac Test integration test apps' runner files against current template app (flutter/flutter#118646) * a788e1b Roll Flutter Engine from 71ee5f19bc16 to 59ea78bfabda (2 revisions) (flutter/flutter#119087) * c35370c Roll Flutter Engine from 59ea78bfabda to 2499a5d9fca7 (2 revisions) (flutter/flutter#119089) * 2f0dd56 Refactor highlight handling in FocusManager (flutter/flutter#119075) * 2759f3f Roll Flutter Engine from 2499a5d9fca7 to d98926c32ee7 (2 revisions) (flutter/flutter#119090) * 760fb21 Roll Flutter Engine from d98926c32ee7 to bec40654a5d7 (2 revisions) (flutter/flutter#119093) * bbca694 Roll Flutter Engine from bec40654a5d7 to 5405f2c26e85 (2 revisions) (flutter/flutter#119095) * 6414c36 f1464b49c Manually roll ANGLE, vulkan-deps, SwiftShader (flutter/engine#38650) (flutter/flutter#119097) * 426cdd9 55bb8deaf [Impeller] Linear sample atlas glyphs when the CTM isn't translation/scale only (flutter/engine#39112) (flutter/flutter#119098) * 83c3a61 Only emit image painting events in debug & profile modes. (flutter/flutter#118872) * b113df2 bffb98352 Roll Skia from b72fececbdcc to 8ffd5c20d634 (3 revisions) (flutter/engine#39114) (flutter/flutter#119099) * 351466a Add Decoding Flutter videos to API docs (flutter/flutter#116454) * 318f875 Pass through magnifierConfiguration (flutter/flutter#118270) * eced23e d39ab638b Roll Fuchsia Mac SDK from MUvFS0baOnigVUIND... to _H53AyDxR9Pm2TbwN... (flutter/engine#39122) (flutter/flutter#119126) * 29ab437 Add Material 3 `CheckboxListTile` example and update existing examples (flutter/flutter#118792) * a815ee6 8efc7183b Roll Skia from 8ffd5c20d634 to da5034f9d117 (4 revisions) (flutter/engine#39123) (flutter/flutter#119129)

…r#3089) * 373523184 Cleanup old Dart SDK layout compatibility (flutter/flutter#118819) * 4d250302a Add leak_tracker as dev_dependency. (flutter/flutter#118952) * e3c51a2f2 Add Windows unit tests to plugin template (flutter/flutter#118638) * d20dd9e4b Roll Flutter Engine from 7d3233d26d09 to 71ee5f19bc16 (15 revisions) (flutter/flutter#119081) * 5dabe102a Fix path name to discover debug apk on add2app builds (flutter/flutter#117999) * 50ed8a34b Enable `unnecessary_null_comparison` check (flutter/flutter#118849) * 455e6aca5 Test integration test apps' runner files against current template app (flutter/flutter#118646) * a788e1b31 Roll Flutter Engine from 71ee5f19bc16 to 59ea78bfabda (2 revisions) (flutter/flutter#119087) * c35370cf0 Roll Flutter Engine from 59ea78bfabda to 2499a5d9fca7 (2 revisions) (flutter/flutter#119089) * 2f0dd5673 Refactor highlight handling in FocusManager (flutter/flutter#119075) * 2759f3f0b Roll Flutter Engine from 2499a5d9fca7 to d98926c32ee7 (2 revisions) (flutter/flutter#119090) * 760fb2115 Roll Flutter Engine from d98926c32ee7 to bec40654a5d7 (2 revisions) (flutter/flutter#119093) * bbca694ef Roll Flutter Engine from bec40654a5d7 to 5405f2c26e85 (2 revisions) (flutter/flutter#119095) * 6414c3604 f1464b49c Manually roll ANGLE, vulkan-deps, SwiftShader (flutter/engine#38650) (flutter/flutter#119097) * 426cdd90c 55bb8deaf [Impeller] Linear sample atlas glyphs when the CTM isn't translation/scale only (flutter/engine#39112) (flutter/flutter#119098) * 83c3a61e3 Only emit image painting events in debug & profile modes. (flutter/flutter#118872) * b113df2dc bffb98352 Roll Skia from b72fececbdcc to 8ffd5c20d634 (3 revisions) (flutter/engine#39114) (flutter/flutter#119099) * 351466aea Add Decoding Flutter videos to API docs (flutter/flutter#116454) * 318f8758b Pass through magnifierConfiguration (flutter/flutter#118270) * eced23eab d39ab638b Roll Fuchsia Mac SDK from MUvFS0baOnigVUIND... to _H53AyDxR9Pm2TbwN... (flutter/engine#39122) (flutter/flutter#119126) * 29ab437e2 Add Material 3 `CheckboxListTile` example and update existing examples (flutter/flutter#118792) * a815ee634 8efc7183b Roll Skia from 8ffd5c20d634 to da5034f9d117 (4 revisions) (flutter/engine#39123) (flutter/flutter#119129)

yaakovschectman added platform-windows Building on or for Windows specifically a: desktop Running on desktop labels Jan 17, 2023

yaakovschectman requested review from cbracken, loic-sharma and christopherfujino January 17, 2023 22:04

yaakovschectman self-assigned this Jan 17, 2023

flutter-dashboard bot added the team Infra upgrades, team productivity, code health, technical debt. See also team: labels. label Jan 17, 2023

yaakovschectman commented Jan 17, 2023

View reviewed changes

yaakovschectman force-pushed the template_test branch from dd983c8 to 46bd0e8 Compare January 18, 2023 15:04

yaakovschectman marked this pull request as ready for review January 18, 2023 15:20

yaakovschectman requested a review from keyonghan as a code owner January 18, 2023 15:20

loic-sharma reviewed Jan 18, 2023

View reviewed changes

dev/devicelab/bin/tasks/windows_app_template_test.dart Outdated Show resolved Hide resolved

loic-sharma reviewed Jan 18, 2023

View reviewed changes

cbracken reviewed Jan 18, 2023

View reviewed changes

christopherfujino reviewed Jan 18, 2023

View reviewed changes

loic-sharma reviewed Jan 18, 2023

View reviewed changes

dev/bots/analyze.dart Outdated Show resolved Hide resolved

yaakovschectman requested review from cbracken, loic-sharma and christopherfujino January 19, 2023 17:07

loic-sharma reviewed Jan 19, 2023

View reviewed changes

yaakovschectman force-pushed the template_test branch 2 times, most recently from 453d786 to e5cbd95 Compare January 20, 2023 21:38

yaakovschectman added 6 commits January 23, 2023 16:08

Create template file test

6f92342

Add to .ci.yaml

83a8b00

Add to TESTOWNERS

3d318ee

Organize test script

efc54a0

Push license to top of file

a119f4c

Equals sign

e8969da

yaakovschectman and others added 10 commits January 23, 2023 16:08

Utilize path.join

57d9f0d

Expand error message

69b4427

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

Fix missing newline string

068637a

Move template file test to analyze.dart

85675b0

Fix newline

6937e45

Var name

5b55be3

Fix extension length

d938484

Use ignore-list for filenames

0dadf11

Update dev/bots/analyze.dart

3681f8a

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

Indentation

83673b6

yaakovschectman force-pushed the template_test branch from e5cbd95 to 83673b6 Compare January 23, 2023 21:09

cbracken approved these changes Jan 24, 2023

View reviewed changes

yaakovschectman requested a review from loic-sharma January 24, 2023 18:33

loic-sharma approved these changes Jan 24, 2023

View reviewed changes

yaakovschectman merged commit 455e6ac into flutter:master Jan 24, 2023

yaakovschectman deleted the template_test branch January 24, 2023 22:00

This was referenced Jan 25, 2023

Roll Flutter from c35efdaa6854 to a815ee634202 (22 revisions) flutter/packages#3089

Merged

Roll Flutter from c35efdaa6854 to a815ee634202 (22 revisions) flutter/plugins#7025

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 25, 2023

455e6aca5 Test integration test apps' runner files against current te…

66d5695

…mplate app (flutter/flutter#118646)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 25, 2023

455e6ac Test integration test apps' runner files against current temp…

dfb54c8

…late app (flutter/flutter#118646)

tgucio mentioned this pull request Jan 27, 2023

Avoid null terminating characters in strings from Utf8FromUtf16() #109729

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test integration test apps' runner files against current template app #118646

Test integration test apps' runner files against current template app #118646

Test integration test apps' runner files against current template app #118646

Test integration test apps' runner files against current template app #118646

Conversation

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment