Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion dev/bots/test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ Future<Null> _runFlutterTest(String workingDirectory, {
workingDirectory: workingDirectory,
expectFailure: expectFailure,
printOutput: printOutput,
skip: skip || Platform.isWindows, // TODO(goderbauer): run on Windows when sky_shell is available
skip: skip,
);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/flutter/test/animation/tween_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ void main() {
);
final Matrix4 c = a.clone()..rotateZ(1.0);
final Matrix4Tween rotationTween = new Matrix4Tween(begin: a, end: c);
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this differ on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

expect(
rotationTween.lerp(0.5).absoluteError(
a.clone()..rotateZ(0.5)
Expand Down
10 changes: 6 additions & 4 deletions packages/flutter/test/foundation/error_reporting_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import 'dart:async';
import 'package:flutter/foundation.dart';
import 'package:test/test.dart';

import 'platform_utils.dart';

dynamic getAssertionErrorWithMessage() {
try {
assert(false, 'Message goes here.');
Expand Down Expand Up @@ -62,7 +64,7 @@ Future<Null> main() async {
information.writeln('line 2 of extra information\n'); // the double trailing newlines here are intentional
},
));
expect(console.join('\n'), matches(new RegExp(
expect(sanitizePaths(console.join('\n')), matches(new RegExp(
'^══╡ EXCEPTION CAUGHT BY ERROR HANDLING TEST ╞═══════════════════════════════════════════════════════\n'
'The following assertion was thrown testing the error handling logic:\n'
'Message goes here\\.\n'
Expand Down Expand Up @@ -99,7 +101,7 @@ Future<Null> main() async {
FlutterError.dumpErrorToConsole(new FlutterErrorDetails(
exception: getAssertionErrorWithLongMessage(),
));
expect(console.join('\n'), matches(new RegExp(
expect(sanitizePaths(console.join('\n')), matches(new RegExp(
'^══╡ EXCEPTION CAUGHT BY FLUTTER FRAMEWORK ╞═════════════════════════════════════════════════════════\n'
'The following assertion was thrown:\n'
'word word word word word word word word word word word word word word word word word word word word '
Expand Down Expand Up @@ -144,7 +146,7 @@ Future<Null> main() async {
information.writeln('line 2 of extra information\n'); // the double trailing newlines here are intentional
},
));
expect(console.join('\n'), matches(new RegExp(
expect(sanitizePaths(console.join('\n')), matches(new RegExp(
'^══╡ EXCEPTION CAUGHT BY ERROR HANDLING TEST ╞═══════════════════════════════════════════════════════\n'
'The following assertion was thrown testing the error handling logic:\n'
'\'[^\']+flutter/test/foundation/error_reporting_test\\.dart\': Failed assertion: line [0-9]+ pos [0-9]+: \'false\': is not true\\.\n'
Expand All @@ -170,7 +172,7 @@ Future<Null> main() async {
FlutterError.dumpErrorToConsole(new FlutterErrorDetails(
exception: getAssertionErrorWithoutMessage(),
));
expect(console.join('\n'), matches('Another exception was thrown: \'[^\']+flutter/test/foundation/error_reporting_test\\.dart\': Failed assertion: line [0-9]+ pos [0-9]+: \'false\': is not true\\.'));
expect(sanitizePaths(console.join('\n')), matches('Another exception was thrown: \'[^\']+flutter/test/foundation/error_reporting_test\\.dart\': Failed assertion: line [0-9]+ pos [0-9]+: \'false\': is not true\\.'));
console.clear();
FlutterError.resetErrorCount();
});
Expand Down
12 changes: 12 additions & 0 deletions packages/flutter/test/foundation/platform_utils.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:io';

String sanitizePaths(String path) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a doc comment here?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

if (Platform.isWindows) {
path = path.replaceAll(new RegExp('\\\\'), '/');
}
return path;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add newline at end of file

6 changes: 4 additions & 2 deletions packages/flutter/test/foundation/stack_trace_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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


test('FlutterError.defaultStackFilter', () {
final List<String> filtered = FlutterError.defaultStackFilter(StackTrace.current.toString().trimRight().split('\n')).toList();
final List<String> filtered = FlutterError.defaultStackFilter(sanitizePaths(StackTrace.current.toString()).trimRight().split('\n')).toList();
expect(filtered.length, greaterThanOrEqualTo(4));
expect(filtered[0], matches(r'^#0 +main\.<anonymous closure> \(.*stack_trace_test\.dart:[0-9]+:[0-9]+\)$'));
expect(filtered[1], matches(r'^#1 +Declarer\.test\.<anonymous closure>.<anonymous closure> \(package:test/.+:[0-9]+:[0-9]+\)$'));
Expand All @@ -18,7 +20,7 @@ void main() {
});

test('FlutterError.defaultStackFilter (async test body)', () async {
final List<String> filtered = FlutterError.defaultStackFilter(StackTrace.current.toString().trimRight().split('\n')).toList();
final List<String> filtered = FlutterError.defaultStackFilter(sanitizePaths(StackTrace.current.toString()).trimRight().split('\n')).toList();
expect(filtered.length, greaterThanOrEqualTo(3));
expect(filtered[0], matches(r'^#0 +main\.<anonymous closure> \(.*stack_trace_test\.dart:[0-9]+:[0-9]+\)$'));
expect(filtered[1], equals('<asynchronous suspension>'));
Expand Down
24 changes: 16 additions & 8 deletions packages/flutter/test/painting/text_painter_rtl_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:io';

import 'package:flutter/foundation.dart';
import 'package:flutter/painting.dart';
import 'package:flutter_test/flutter_test.dart';
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

final for offset and const for Offset?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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);

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

expect( // between A and Alef, before the Alef
painter.getOffsetForCaret(const TextPosition(offset: 1, affinity: TextAffinity.downstream), Rect.zero),
const Offset(10.0, 10.0),
offset,
);

offset = (Platform.isWindows) ? new Offset(0.0, 12.0) : new Offset(0.0, 10.0);
expect( // after the Alef
painter.getOffsetForCaret(const TextPosition(offset: 2, affinity: TextAffinity.upstream), Rect.zero),
const Offset(0.0, 10.0),
offset,
);
expect( // after the Alef
painter.getOffsetForCaret(const TextPosition(offset: 2, affinity: TextAffinity.downstream), Rect.zero),
const Offset(0.0, 10.0),
offset,
);

TextBox textBox = (Platform.isWindows) ?
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

expect(
painter.getBoxesForSelection(const TextSelection(baseOffset: 0, extentOffset: 2)),
const <TextBox>[
const TextBox.fromLTRBD(0.0, 0.0, 10.0, 10.0, TextDirection.ltr), // A
const TextBox.fromLTRBD(0.0, 10.0, 10.0, 20.0, TextDirection.rtl), // Alef
<TextBox>[
new TextBox.fromLTRBD(0.0, 0.0, 10.0, 10.0, TextDirection.ltr), // A
textBox, // Alef
],
);
expect(
Expand All @@ -313,8 +321,8 @@ void main() {
);
expect(
painter.getBoxesForSelection(const TextSelection(baseOffset: 1, extentOffset: 2)),
const <TextBox>[
const TextBox.fromLTRBD(0.0, 10.0, 10.0, 20.0, TextDirection.rtl), // Alef
<TextBox>[
textBox, // Alef
],
);
});
Expand Down
12 changes: 9 additions & 3 deletions packages/flutter/test/rendering/paragraph_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:io';
import 'dart:ui' as ui show TextBox;

import 'package:flutter/rendering.dart';
Expand Down Expand Up @@ -67,7 +68,10 @@ void main() {
);

expect(boxes.any((ui.TextBox box) => box.left == 250 && box.top == 0), isTrue);
expect(boxes.any((ui.TextBox box) => box.right == 100 && box.top == 10), isTrue);
expect(boxes.any((ui.TextBox box) =>
Platform.isWindows ?
box.right == 100 && box.top == 13 :
box.right == 100 && box.top == 10), isTrue);
Copy link
Contributor

Choose a reason for hiding this comment

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

this also seems wrong

});

test('getWordBoundary control test', () {
Expand Down Expand Up @@ -180,7 +184,8 @@ void main() {
}

layoutAt(null);
expect(paragraph.size.height, 130.0);
double height = (Platform.isWindows) ? 138.0 : 130.0;
expect(paragraph.size.height, height);

layoutAt(1);
expect(paragraph.size.height, 10.0);
Expand All @@ -189,7 +194,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);
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you missed these when switching to skipping tests instead

});

test('changing color does not do layout', () {
Expand Down
58 changes: 32 additions & 26 deletions packages/flutter_tools/test/commands/test_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,37 @@ void main() {
final String automatedTestsDirectory = fs.path.join('..', '..', 'dev', 'automated_tests');
final String flutterTestDirectory = fs.path.join(automatedTestsDirectory, 'flutter_test');

testUsingContext('report nice errors for exceptions thrown within testWidgets()', () async {
Cache.flutterRoot = '../..';
return _testFile('exception_handling', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when a guarded function was called without await', () async {
Cache.flutterRoot = '../..';
return _testFile('test_async_utils_guarded', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when an async function was called without await', () async {
Cache.flutterRoot = '../..';
return _testFile('test_async_utils_unguarded', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when a Ticker is left running', () async {
Cache.flutterRoot = '../..';
return _testFile('ticker', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when a pubspec.yaml is missing a flutter_test dependency', () async {
final String missingDependencyTests = fs.path.join('..', '..', 'dev', 'missing_dependency_tests');
Cache.flutterRoot = '../..';
return _testFile('trivial', missingDependencyTests, missingDependencyTests);
});
// TODO(bkonyi): These tests all throw exceptions with unicode characters used for formatting,
Copy link
Member

Choose a reason for hiding this comment

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

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]

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// but the unicode characters aren't being reported correctly on stderr from Process.run. This
// seems like it's something to do with how the Dart VM handles Unicode on Windows, so we'll
// skip these for now.
if (!io.Platform.isWindows) {
testUsingContext('report nice errors for exceptions thrown within testWidgets()', () async {
Cache.flutterRoot = '../..';
return _testFile('exception_handling', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when a guarded function was called without await', () async {
Cache.flutterRoot = '../..';
return _testFile('test_async_utils_guarded', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when an async function was called without await', () async {
Cache.flutterRoot = '../..';
return _testFile('test_async_utils_unguarded', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when a Ticker is left running', () async {
Cache.flutterRoot = '../..';
return _testFile('ticker', automatedTestsDirectory, flutterTestDirectory);
});

testUsingContext('report a nice error when a pubspec.yaml is missing a flutter_test dependency', () async {
final String missingDependencyTests = fs.path.join('..', '..', 'dev', 'missing_dependency_tests');
Cache.flutterRoot = '../..';
return _testFile('trivial', missingDependencyTests, missingDependencyTests);
});
}

testUsingContext('run a test when its name matches a regexp', () async {
Cache.flutterRoot = '../..';
Expand Down Expand Up @@ -82,7 +88,7 @@ void main() {
expect(result.exitCode, 0);
});

}, skip: io.Platform.isWindows); // TODO(goderbauer): enable when sky_shell is available
});
}

Future<Null> _testFile(String testName, String workingDirectory, String testDirectory) async {
Expand Down