Enabled flutter_tools tests that were previously disabled on Windows. #13487

bkonyi · 2017-12-11T20:09:51Z

No description provided.

goderbauer · 2017-12-11T20:54:26Z

There is also

flutter/packages/flutter_tools/test/commands/create_test.dart

Line 212 in aedee0d

// TODO(pq): enable when sky_shell is available

, which should probably be enabled.

goderbauer · 2017-12-11T20:56:10Z

There is also #13486, which I am happy to drop if you want to include it into this one.

tvolkert · 2017-12-12T16:30:20Z

dev/bots/test.dart should also remove the windows skip check.

goderbauer

So close! :) appveyor is reporting one more test as failing. Can you take a look at that one?

goderbauer · 2017-12-14T23:46:31Z

packages/flutter/test/foundation/platform_utils.dart

+
+import 'dart:io';
+
+String sanitizePaths(String path) {


Can you add a doc comment here?

goderbauer · 2017-12-14T23:49:27Z

packages/flutter_tools/test/commands/test_test.dart

-      Cache.flutterRoot = '../..';
-      return _testFile('trivial', missingDependencyTests, missingDependencyTests);
-    });
+    // TODO(bkonyi): These tests all throw exceptions with unicode characters used for formatting,


Can you mark them individually as skipped via [1]? That way, they'll get reported as skipped by the test runner. Also might be good to file an issue about this and attaching it to the TODO.

[1]

flutter/packages/flutter_tools/test/src/context.dart

Line 72 in 0350c9e

bool skip, // should default to `false`, but https://github.com/dart-lang/test/issues/545 doesn't allow this

to add to @goderbauer's comments: in general, please prefer using skip: ... rather than an if statement, because skipped tests count towards our technical debt, whereas if statements do not.

Ah, didn't realize that was a parameter. Will do.

goderbauer · 2017-12-14T23:51:28Z

packages/flutter/test/painting/text_painter_rtl_test.dart

@@ -284,25 +286,31 @@ void main() {
      painter.getOffsetForCaret(const TextPosition(offset: 1, affinity: TextAffinity.upstream), Rect.zero),
      const Offset(10.0, 0.0),
    );
+
+    Offset offset = (Platform.isWindows) ? new Offset(7.0, 12.0) : new Offset(10.0, 10.0);


final for offset and const for Offset?

what is going on here? This seems extremely wrong. The Ahem font is exactly 1em square for every glyph. If this is not coming out as 10,10, then there's a real bug.

@goderbauer, using const for Offset doesn't seem to work since Platform.isWindows isn't const.

@Hixie, I'm not 100% sure as I'm not familiar with this code. However, I was running into issues with Windows rendering fonts differently than other platforms which broke tests that were looking for specific pixel values, so I assumed that was the case here.

This is what I meant, this doesn't work?

final Offset offset = (Platform.isWindows) ? const Offset(7.0, 12.0) : const Offset(10.0, 10.0);

The analyzer won't let you do anything but what @goderbauer suggests. :-)

Hm, I'll have to check again... I might have been trying something like this:

const Offset offset = (Platform.isWindows) ? const Offset(...) : const Offset(...);

goderbauer · 2017-12-14T23:52:09Z

packages/flutter/test/painting/text_painter_rtl_test.dart

    );

+    TextBox textBox = (Platform.isWindows) ?


final for textBox - and does TextBox have a const constructor that could be used?

Using const for TextBox doesn't seem to work since Platform.isWindows isn't const.

Hixie · 2017-12-15T20:59:46Z

packages/flutter/test/animation/tween_test.dart

-    expect(rotationTween.lerp(0.0), equals(a));
-    expect(rotationTween.lerp(1.0), equals(c));
+    expect(rotationTween.lerp(0.0).absoluteError(a), moreOrLessEquals(0.0));
+    expect(rotationTween.lerp(1.0).absoluteError(c), moreOrLessEquals(0.0));


why does this differ on windows?

There was some issues with floating point errors on Windows (differences on the magnitude of 1e-16 for individual entries in the matrix) which was causing the equals check to fail. I'm actually surprised the original worked consistently before, since testing floats for equality is just asking for trouble.

It's really weird that Mac and Linux would be consistent here but Windows would not. I think this points to a bug somewhere. I don't think we should check this in without better understanding what's going on.

Maybe a compiler somewhere is ordering the multiplications differently or something?

Hixie · 2017-12-15T21:00:38Z

packages/flutter/test/foundation/stack_trace_test.dart

@@ -5,11 +5,13 @@
 import 'package:flutter/foundation.dart';
 import 'package:test/test.dart';

+import 'platform_utils.dart';
+
 void main() {
  // TODO(8128): These tests and the filtering mechanism should be revisited to account for causal async stack traces.


please while you're here fix this TODO to be the right syntax ("TODO(username): description, https://bug/url")

Hixie · 2017-12-15T21:01:37Z

packages/flutter/test/painting/text_painter_rtl_test.dart

    );

+    TextBox textBox = (Platform.isWindows) ?
+                      new TextBox.fromLTRBD(0.0, 12.0, 7.0, 22.0, TextDirection.rtl) :
+                      new TextBox.fromLTRBD(0.0, 10.0, 10.0, 20.0, TextDirection.rtl);


Hixie · 2017-12-15T21:02:03Z

packages/flutter/test/rendering/paragraph_test.dart

+    expect(boxes.any((ui.TextBox box) =>
+      Platform.isWindows ?
+      box.right == 100 && box.top == 13 :
+      box.right == 100 && box.top == 10), isTrue);


this also seems wrong

Hixie · 2017-12-15T22:10:16Z

@Hixie, I'm not 100% sure as I'm not familiar with this code. However, I was running into issues with Windows rendering fonts differently than other platforms which broke tests that were looking for specific pixel values, so I assumed that was the case here.

The font we use for all the tests is a special font that has extremely specific and well-defined metrics and so any variation you see is probably a bug. The tests that test font metrics should not be any different on different platforms.

Hixie · 2017-12-15T22:12:46Z

(My guess given the values I see is that maybe you're not properly loading the Ahem font on Windows? It looks from the metrics as if you're getting Arial instead.)

bkonyi · 2017-12-15T22:17:44Z

@Hixie, that's good to know. Is there any way to verify the right font is being loaded? Also, would it be worth filing an issue and disabling these tests on Windows in the meantime? That way at least most of the tests will be running on Windows while we investigate your concerns above instead of having no tests running on Windows at all.

Hixie · 2017-12-15T22:19:48Z

@Hixie, that's good to know. Is there any way to verify the right font is being loaded?

If you can render anything, you'll be easily able to tell because the Ahem font makes all ASCII characters look like filled squares.

If you can't render anything, it'll be easy to tell by measuring metrics. All ASCII glyphs in Ahem have a height and width equal to the font size.

Also, would it be worth filing an issue and disabling these tests on Windows in the meantime? That way at least most of the tests will be running on Windows while we investigate your concerns above instead of having no tests running on Windows at all.

Sure.

Hixie · 2017-12-21T00:42:35Z

packages/flutter/test/rendering/paragraph_test.dart

@@ -189,7 +191,8 @@ void main() {
    expect(paragraph.size.height, 20.0);

    layoutAt(3);
-    expect(paragraph.size.height, 30.0);
+    height = (Platform.isWindows) ? 34.0 : 30.0;
+    expect(paragraph.size.height, height);


looks like you missed these when switching to skipping tests instead

Hixie · 2017-12-21T00:42:44Z

tvolkert · 2017-12-21T03:49:07Z

packages/flutter/test/foundation/platform_utils.dart

+    path = path.replaceAll(new RegExp('\\\\'), '/');
+  }
+  return path;
+}


Nit: add newline at end of file

tvolkert · 2017-12-21T03:51:56Z

packages/flutter/test/foundation/platform_utils.dart

+import 'dart:io';
+
+/// Replaces Windows-style path separators with Unix-style separators.
+String sanitizePaths(String path) {


This name left me wondering what it was doing (and thinking that it was weird to map windows-style to unix-style when you're on Windows). Maybe call this something like toPosixPath?

Hixie · 2018-01-03T22:00:29Z

@bkonyi Is this PR still on your radar?

bkonyi · 2018-01-04T15:36:34Z

@Hixie yes it is, but I've had to put it off until next week since I'm traveling (working on a remote Windows machine is painful :( ...). I'm investigating why we're hitting the timeout on Appveyor to see if there's any performance gains to be made.

kostaa · 2018-01-14T15:01:41Z

If I understand correctly this PR will enable running Flutter tests on Windows.
It would be great help for those of us who develop on Windows.
Is there an ETA for this?

bkonyi · 2018-01-16T19:14:05Z

@kostaa this PR is to enable running our Flutter tests on our build infrastructure, but you should be able to run flutter test on Windows to test your own applications since that was enabled sometime last month.

Hixie · 2018-01-17T18:19:43Z

We are now running tests on Windows (as a result of a series of other mostly unrelated PRs that happened to make things work on Windows). This PR may be obsolete.

…ter#13487)" (flutter/engine#16546)

* f49a8b6 Roll src/third_party/skia c03e6982f96f..465864cad5d2 (14 commits) (flutter/engine#16524) * c477c06 Enable verbose logging for shell unittests on Fuchsia (flutter/engine#16526) * a662579 Clear frame references at the end of every CanvasKit frame (flutter/engine#16525) * 3f31ea3 Roll src/third_party/skia 465864cad5d2..21f382c19d76 (6 commits) (flutter/engine#16528) * 38fb6b1 Roll fuchsia/sdk/core/linux-amd64 from 8L7NY... to Bmq1m... (flutter/engine#16529) * 9c0168a Roll fuchsia/sdk/core/mac-amd64 from PMcw3... to 7JkB7... (flutter/engine#16530) * e8a888d Roll src/third_party/skia 21f382c19d76..f83d0346c06a (2 commits) (flutter/engine#16532) * 1e8b331 Roll src/third_party/dart 5244d99a5d4e..5fc031ebc1d7 (42 commits) (flutter/engine#16533) * c4e3ae6 Roll src/third_party/skia f83d0346c06a..88c3793a4eaa (1 commits) (flutter/engine#16534) * 6cdb14e Roll src/third_party/skia 88c3793a4eaa..abefc9c170c9 (1 commits) (flutter/engine#16535) * 975acd8 Roll src/third_party/skia abefc9c170c9..4fe89b4d871d (2 commits) (flutter/engine#16536) * b7424d0 Roll src/third_party/dart 5fc031ebc1d7..30151a654151 (2 commits) (flutter/engine#16537) * 25e8127 Roll src/third_party/skia 4fe89b4d871d..dc2782c380f6 (1 commits) (flutter/engine#16538) * 74fa10c Roll src/third_party/dart 30151a654151..76b18c455e2c (1 commits) (flutter/engine#16539) * 91b8e40 Roll src/third_party/skia dc2782c380f6..cdf2491afa04 (1 commits) (flutter/engine#16540) * 5acf9b1 Roll src/third_party/skia cdf2491afa04..50a490a1a4fb (2 commits) (flutter/engine#16541) * 9897777 Roll src/third_party/skia 50a490a1a4fb..c3b67eb988c8 (4 commits) (flutter/engine#16542) * 78a8909 Use os_log instead of syslog on Apple platforms (flutter/engine#13487) * ea56ad2 libtxt: use a fixture in the benchmarks (flutter/engine#16531) * a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (#13487)" (flutter/engine#16546) * 539f64f [fuchsia] Disable retained layers (flutter/engine#16548) * c3b5072 Expose DPI helper functions for Runner apps to use (flutter/engine#16313) * 5041ff1 support endless trace buffer (flutter/engine#16520) * 6aacf5e Re-land: Use os_log instead of syslog on Apple platforms (flutter/engine#16549) * a5736b8 Roll src/third_party/skia c3b67eb988c8..b1525c721ea6 (4 commits) (flutter/engine#16543) * 49a370f Roll src/third_party/dart 76b18c455e2c..e4c39721c473 (6 commits) (flutter/engine#16544) * 270421c Fix ensureInitializationCompleteAsync callback when already initialized. (#39675) (flutter/engine#16503) * ca02b91 Prevent long flash when switching to Flutter app. (#47903) (flutter/engine#16527) * 44e80fd skiping tests in Safari. LUCI recipe for Mac is ready. this is the only step left for stopping us running unit tests in Safari (flutter/engine#16550) * 5fb0116 iOS platform view gesture blocking policy. (flutter/engine#15940) * e0ebaea Revert "Re-land: Use os_log instead of syslog on Apple platforms (#16549)" (flutter/engine#16558)

…ter#13487)" (flutter/engine#16546)

* f49a8b6 Roll src/third_party/skia c03e6982f96f..465864cad5d2 (14 commits) (flutter/engine#16524) * c477c06 Enable verbose logging for shell unittests on Fuchsia (flutter/engine#16526) * a662579 Clear frame references at the end of every CanvasKit frame (flutter/engine#16525) * 3f31ea3 Roll src/third_party/skia 465864cad5d2..21f382c19d76 (6 commits) (flutter/engine#16528) * 38fb6b1 Roll fuchsia/sdk/core/linux-amd64 from 8L7NY... to Bmq1m... (flutter/engine#16529) * 9c0168a Roll fuchsia/sdk/core/mac-amd64 from PMcw3... to 7JkB7... (flutter/engine#16530) * e8a888d Roll src/third_party/skia 21f382c19d76..f83d0346c06a (2 commits) (flutter/engine#16532) * 1e8b331 Roll src/third_party/dart 5244d99a5d4e..5fc031ebc1d7 (42 commits) (flutter/engine#16533) * c4e3ae6 Roll src/third_party/skia f83d0346c06a..88c3793a4eaa (1 commits) (flutter/engine#16534) * 6cdb14e Roll src/third_party/skia 88c3793a4eaa..abefc9c170c9 (1 commits) (flutter/engine#16535) * 975acd8 Roll src/third_party/skia abefc9c170c9..4fe89b4d871d (2 commits) (flutter/engine#16536) * b7424d0 Roll src/third_party/dart 5fc031ebc1d7..30151a654151 (2 commits) (flutter/engine#16537) * 25e8127 Roll src/third_party/skia 4fe89b4d871d..dc2782c380f6 (1 commits) (flutter/engine#16538) * 74fa10c Roll src/third_party/dart 30151a654151..76b18c455e2c (1 commits) (flutter/engine#16539) * 91b8e40 Roll src/third_party/skia dc2782c380f6..cdf2491afa04 (1 commits) (flutter/engine#16540) * 5acf9b1 Roll src/third_party/skia cdf2491afa04..50a490a1a4fb (2 commits) (flutter/engine#16541) * 9897777 Roll src/third_party/skia 50a490a1a4fb..c3b67eb988c8 (4 commits) (flutter/engine#16542) * 78a8909 Use os_log instead of syslog on Apple platforms (flutter/engine#13487) * ea56ad2 libtxt: use a fixture in the benchmarks (flutter/engine#16531) * a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (#13487)" (flutter/engine#16546) * 539f64f [fuchsia] Disable retained layers (flutter/engine#16548) * c3b5072 Expose DPI helper functions for Runner apps to use (flutter/engine#16313) * 5041ff1 support endless trace buffer (flutter/engine#16520) * 6aacf5e Re-land: Use os_log instead of syslog on Apple platforms (flutter/engine#16549) * a5736b8 Roll src/third_party/skia c3b67eb988c8..b1525c721ea6 (4 commits) (flutter/engine#16543) * 49a370f Roll src/third_party/dart 76b18c455e2c..e4c39721c473 (6 commits) (flutter/engine#16544) * 270421c Fix ensureInitializationCompleteAsync callback when already initialized. (#39675) (flutter/engine#16503) * ca02b91 Prevent long flash when switching to Flutter app. (#47903) (flutter/engine#16527) * 44e80fd skiping tests in Safari. LUCI recipe for Mac is ready. this is the only step left for stopping us running unit tests in Safari (flutter/engine#16550) * 5fb0116 iOS platform view gesture blocking policy. (flutter/engine#15940) * e0ebaea Revert "Re-land: Use os_log instead of syslog on Apple platforms (#16549)" (flutter/engine#16558) * 8a6b949 [Fuchsia] Dump syslog output after tests have run (flutter/engine#16561) * bca879c Roll src/third_party/dart e4c39721c473..0299903f3e78 (31 commits) (flutter/engine#16553) * cd11d7a Roll fuchsia/sdk/core/mac-amd64 from 7JkB7... to t4kck... (flutter/engine#16555) * 99a265b [web] Fix edge cases in Paragraph.getPositionForOffset to match Flutter (flutter/engine#16557) * 8f8af1f Update felt documentation (flutter/engine#16559) * 13dce50 Roll src/third_party/skia b1525c721ea6..67da665c27ff (32 commits) (flutter/engine#16562) * 7c67573 Fix multiline Javadoc code blocks (flutter/engine#16565) * aece5ad Move log_listener call into the reboot trap (flutter/engine#16564) * 42f18d9 Roll src/third_party/skia 67da665c27ff..886e8500a9f2 (3 commits) (flutter/engine#16566) * c4c6ef6 Samsung keyboard duplication workaround: updateSelection (flutter/engine#16547) * 15062ca Revert "Re-arm timer as necessary in MessageLoopFuchsia" (flutter/engine#16568) * 8802a1d Roll src/third_party/skia 886e8500a9f2..9102c86a81ad (1 commits) (flutter/engine#16570) * dbdcae4 Roll src/third_party/skia 9102c86a81ad..6029cbd560b7 (2 commits) (flutter/engine#16575) * f39bc73 Exposes FlutterSurfaceView, and FlutterTextureView to FlutterActivity and FlutterFragment. (#41984, #47557) (flutter/engine#16552) * db030ec Roll src/third_party/skia 6029cbd560b7..1a733b5b760a (1 commits) (flutter/engine#16577) * 050d29d Roll src/third_party/skia 1a733b5b760a..1d1333fcedf8 (3 commits) (flutter/engine#16578) * 97fd898 Roll fuchsia/sdk/core/mac-amd64 from t4kck... to oHa-O... (flutter/engine#16581) * 2e67866 Roll src/third_party/skia 1d1333fcedf8..3bf3b92dfab0 (1 commits) (flutter/engine#16584)

Enabled flutter_tools tests that were previously disabled on Windows.

6762d04

bkonyi requested a review from goderbauer December 11, 2017 20:09

googlebot added the cla: yes label Dec 11, 2017

Updated tests to run on Windows now that flutter test is supported.

215d763

goderbauer approved these changes Dec 14, 2017

View reviewed changes

Hixie reviewed Dec 15, 2017

View reviewed changes

Updated tests to address comments from CR.

2575145

Hixie reviewed Dec 21, 2017

View reviewed changes

tvolkert reviewed Dec 21, 2017

View reviewed changes

bkonyi closed this Jan 17, 2018

bkonyi deleted the enable_windows_tests branch January 17, 2018 23:43

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

a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (flut…

9a1a4f8

…ter#13487)" (flutter/engine#16546)

This was referenced Feb 12, 2020

Roll engine 0e8ed2715a35..5041ff1b1e37 (23 commits) #50667

Closed

Roll engine 0e8ed2715a35..e0ebaea59071 (31 commits) #50671

Merged

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

a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (flut…

80f0815

…ter#13487)" (flutter/engine#16546)

engine-flutter-autoroll mentioned this pull request Feb 14, 2020

Roll engine 0e8ed2715a35..2e6786621f7b (49 commits) #50803

Merged

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

a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (flut…

5300f57

…ter#13487)" (flutter/engine#16546)

cbracken mentioned this pull request Apr 2, 2021

Use unified logging instead of syslog on iOS 13+ #44030

Open

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021

Enabled flutter_tools tests that were previously disabled on Windows. #13487

Enabled flutter_tools tests that were previously disabled on Windows. #13487

Conversation

bkonyi commented Dec 11, 2017

goderbauer commented Dec 11, 2017

goderbauer commented Dec 11, 2017

tvolkert commented Dec 12, 2017

goderbauer left a 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

Hixie commented Dec 15, 2017

Hixie commented Dec 15, 2017

bkonyi commented Dec 15, 2017

Hixie commented Dec 15, 2017

Choose a reason for hiding this comment

Hixie commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Jan 3, 2018

bkonyi commented Jan 4, 2018

kostaa commented Jan 14, 2018

bkonyi commented Jan 16, 2018

Hixie commented Jan 17, 2018