From 7861d029433dca197f5453c22e20155b29556c50 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Wed, 13 Apr 2016 13:53:39 -0700 Subject: [PATCH] Fix dependency skew. (#3306) ...by adding tests to our examples that don't import flutter_test, which pins the relevant dependencies. Also, provide more information when complaining about leaked transient callbacks in tests. Also, make tests display full information when they have an exception, by bypassing the throttling we have for Android logging in tests. Also, make the word wrapping not wrap stack traces if they happen to be included in exception output. Also, fix a leaked transient callback in the checkbox code. --- dev/manual_tests/pubspec.yaml | 4 + .../test/card_collection_test.dart | 42 +++++++ examples/hello_world/pubspec.yaml | 4 + examples/hello_world/test/hello_test.dart | 19 ++++ examples/layers/pubspec.yaml | 4 + examples/layers/test/sector_test.dart | 12 ++ examples/material_gallery/pubspec.yaml | 5 + .../material_gallery/test/smoke_test.dart | 62 +++++++++++ .../flutter/lib/src/material/toggleable.dart | 32 ++++++ .../flutter/lib/src/scheduler/scheduler.dart | 104 ++++++++++++++++-- .../flutter/lib/src/scheduler/ticker.dart | 6 +- packages/flutter/lib/src/services/print.dart | 33 +++++- .../flutter_test/lib/src/widget_tester.dart | 30 +++-- travis/test.sh | 4 + 14 files changed, 330 insertions(+), 31 deletions(-) create mode 100644 dev/manual_tests/test/card_collection_test.dart create mode 100644 examples/hello_world/test/hello_test.dart create mode 100644 examples/layers/test/sector_test.dart create mode 100644 examples/material_gallery/test/smoke_test.dart diff --git a/dev/manual_tests/pubspec.yaml b/dev/manual_tests/pubspec.yaml index b3708d8307..ab13b22646 100644 --- a/dev/manual_tests/pubspec.yaml +++ b/dev/manual_tests/pubspec.yaml @@ -2,3 +2,7 @@ name: flutter_manual_tests dependencies: flutter: path: ../../packages/flutter +dev_dependencies: + test: any # flutter_test provides the version constraints + flutter_test: + path: ../../packages/flutter_test diff --git a/dev/manual_tests/test/card_collection_test.dart b/dev/manual_tests/test/card_collection_test.dart new file mode 100644 index 0000000000..e574a44984 --- /dev/null +++ b/dev/manual_tests/test/card_collection_test.dart @@ -0,0 +1,42 @@ +// Copyright 2016 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 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:test/test.dart'; + +import '../card_collection.dart' as card_collection; + +void main() { + test("Card Collection smoke test", () { + testWidgets((WidgetTester tester) { + card_collection.main(); // builds the app and schedules a frame but doesn't trigger one + tester.pump(); // see https://github.com/flutter/flutter/issues/1865 + tester.pump(); // triggers a frame + + Element navigationMenu = tester.findElement((Element element) { + Widget widget = element.widget; + if (widget is Tooltip) + return widget.message == 'Open navigation menu'; + return false; + }); + + expect(navigationMenu, isNotNull); + + tester.tap(navigationMenu); + tester.pump(); // start opening menu + tester.pump(const Duration(seconds: 1)); // wait til it's really opened + + // smoke test for various checkboxes + tester.tap(tester.findText('Make card labels editable')); + tester.pump(); + tester.tap(tester.findText('Let the sun shine')); + tester.pump(); + tester.tap(tester.findText('Make card labels editable')); + tester.pump(); + tester.tap(tester.findText('Vary font sizes')); + tester.pump(); + }); + }); +} diff --git a/examples/hello_world/pubspec.yaml b/examples/hello_world/pubspec.yaml index a61659d2b3..0c9b34fcf8 100644 --- a/examples/hello_world/pubspec.yaml +++ b/examples/hello_world/pubspec.yaml @@ -2,3 +2,7 @@ name: hello_world dependencies: flutter: path: ../../packages/flutter +dev_dependencies: + test: any # flutter_test provides the version constraints + flutter_test: + path: ../../packages/flutter_test diff --git a/examples/hello_world/test/hello_test.dart b/examples/hello_world/test/hello_test.dart new file mode 100644 index 0000000000..b7f595253d --- /dev/null +++ b/examples/hello_world/test/hello_test.dart @@ -0,0 +1,19 @@ +// Copyright 2016 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 'package:flutter_test/flutter_test.dart'; +import 'package:test/test.dart'; + +import '../lib/main.dart' as hello_world; + +void main() { + test("Hello world smoke test", () { + testWidgets((WidgetTester tester) { + hello_world.main(); // builds the app and schedules a frame but doesn't trigger one + tester.pump(); // triggers a frame + + expect(tester.findText('Hello, world!'), isNotNull); + }); + }); +} diff --git a/examples/layers/pubspec.yaml b/examples/layers/pubspec.yaml index eda3add84d..2eb57a724f 100644 --- a/examples/layers/pubspec.yaml +++ b/examples/layers/pubspec.yaml @@ -2,3 +2,7 @@ name: flutter_examples_layers dependencies: flutter: path: ../../packages/flutter +dev_dependencies: + test: any # flutter_test provides the version constraints + flutter_test: + path: ../../packages/flutter_test diff --git a/examples/layers/test/sector_test.dart b/examples/layers/test/sector_test.dart new file mode 100644 index 0000000000..078d225e6f --- /dev/null +++ b/examples/layers/test/sector_test.dart @@ -0,0 +1,12 @@ +// Copyright 2015 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 '../rendering/src/sector_layout.dart'; +import 'package:test/test.dart'; + +void main() { + test('SectorConstraints', () { + expect(const SectorConstraints().isTight, isFalse); + }); +} diff --git a/examples/material_gallery/pubspec.yaml b/examples/material_gallery/pubspec.yaml index 6ef8144531..38040c26ad 100644 --- a/examples/material_gallery/pubspec.yaml +++ b/examples/material_gallery/pubspec.yaml @@ -11,3 +11,8 @@ dependencies: flutter_markdown: path: ../../packages/flutter_markdown flutter_gallery_assets: '0.0.15' + +dev_dependencies: + test: any # flutter_test provides the version constraints + flutter_test: + path: ../../packages/flutter_test diff --git a/examples/material_gallery/test/smoke_test.dart b/examples/material_gallery/test/smoke_test.dart new file mode 100644 index 0000000000..a65c10d4a9 --- /dev/null +++ b/examples/material_gallery/test/smoke_test.dart @@ -0,0 +1,62 @@ +// Copyright 2016 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 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:test/test.dart'; + +import '../lib/main.dart' as material_gallery; + +void main() { + test('Material Gallery app smoke test', () { + testWidgets((WidgetTester tester) { + material_gallery.main(); // builds the app and schedules a frame but doesn't trigger one + tester.pump(); // see https://github.com/flutter/flutter/issues/1865 + tester.pump(); // triggers a frame + + // Try loading Weather demo + tester.tap(tester.findText('Demos')); + tester.pump(); + tester.pump(const Duration(seconds: 1)); // wait til it's really opened + + tester.tap(tester.findText('Weather')); + tester.pump(); + tester.pump(const Duration(seconds: 1)); // wait til it's really opened + + // Go back + Element backButton = tester.findElement((Element element) { + Widget widget = element.widget; + if (widget is Tooltip) + return widget.message == 'Back'; + return false; + }); + expect(backButton, isNotNull); + tester.tap(backButton); + tester.pump(); // start going back + tester.pump(const Duration(seconds: 1)); // wait til it's finished + + // Open menu + Element navigationMenu = tester.findElement((Element element) { + Widget widget = element.widget; + if (widget is Tooltip) + return widget.message == 'Open navigation menu'; + return false; + }); + expect(navigationMenu, isNotNull); + tester.tap(navigationMenu); + tester.pump(); // start opening menu + tester.pump(const Duration(seconds: 1)); // wait til it's really opened + + // switch theme + tester.tap(tester.findText('Dark')); + tester.pump(); + tester.pump(const Duration(seconds: 1)); // wait til it's changed + + // switch theme + tester.tap(tester.findText('Light')); + tester.pump(); + tester.pump(const Duration(seconds: 1)); // wait til it's changed + }); + }); +} diff --git a/packages/flutter/lib/src/material/toggleable.dart b/packages/flutter/lib/src/material/toggleable.dart index 31203795c0..2dc8f4e5bf 100644 --- a/packages/flutter/lib/src/material/toggleable.dart +++ b/packages/flutter/lib/src/material/toggleable.dart @@ -117,6 +117,38 @@ abstract class RenderToggleable extends RenderConstrainedBox implements Semantic TapGestureRecognizer _tap; Point _downPosition; + @override + void attach(PipelineOwner owner) { + super.attach(owner); + if (_positionController != null) { + if (value) + _positionController.forward(); + else + _positionController.reverse(); + } + if (_reactionController != null && isInteractive) { + switch (_reactionController.status) { + case AnimationStatus.forward: + _reactionController.forward(); + break; + case AnimationStatus.reverse: + _reactionController.reverse(); + break; + case AnimationStatus.dismissed: + case AnimationStatus.completed: + // nothing to do + break; + } + } + } + + @override + void detach() { + _positionController?.stop(); + _reactionController?.stop(); + super.detach(); + } + void _handlePositionStateChanged(AnimationStatus status) { if (isInteractive) { if (status == AnimationStatus.completed && !_value) diff --git a/packages/flutter/lib/src/scheduler/scheduler.dart b/packages/flutter/lib/src/scheduler/scheduler.dart index 361a2e9a2a..b369ee7e04 100644 --- a/packages/flutter/lib/src/scheduler/scheduler.dart +++ b/packages/flutter/lib/src/scheduler/scheduler.dart @@ -30,10 +30,26 @@ typedef bool SchedulingStrategy({ int priority, Scheduler scheduler }); /// /// Combines the task and its priority. class _TaskEntry { + const _TaskEntry(this.task, this.priority); final VoidCallback task; final int priority; +} - const _TaskEntry(this.task, this.priority); +class _FrameCallbackEntry { + _FrameCallbackEntry(this.callback, { bool rescheduling: false }) { + assert(() { + if (rescheduling) { + assert(currentCallbackStack != null); + stack = currentCallbackStack; + } else { + stack = StackTrace.current; + } + return true; + }); + } + static StackTrace currentCallbackStack; + final FrameCallback callback; + StackTrace stack; } class Priority { @@ -141,7 +157,7 @@ abstract class Scheduler extends BindingBase { } int _nextFrameCallbackId = 0; // positive - Map _transientCallbacks = {}; + Map _transientCallbacks = {}; final Set _removedIds = new HashSet(); int get transientCallbackCount => _transientCallbacks.length; @@ -150,9 +166,14 @@ abstract class Scheduler extends BindingBase { /// /// Adds the given callback to the list of frame-callbacks and ensures that a /// frame is scheduled. - int scheduleFrameCallback(FrameCallback callback) { + /// + /// If `rescheduling` is true, the call must be in the context of a + /// frame callback, and for debugging purposes the stack trace + /// stored for this callback will be the same stack trace as for the + /// current callback. + int scheduleFrameCallback(FrameCallback callback, { bool rescheduling: false }) { _ensureBeginFrameCallback(); - return addFrameCallback(callback); + return addFrameCallback(callback, rescheduling: rescheduling); } /// Adds a frame callback. @@ -162,9 +183,18 @@ abstract class Scheduler extends BindingBase { /// /// The registered callbacks are executed in the order in which they have been /// registered. - int addFrameCallback(FrameCallback callback) { + /// + /// Callbacks registered with this method will not be invoked until + /// a frame is requested. To register a callback and ensure that a + /// frame is immediately scheduled, use [scheduleFrameCallback]. + /// + /// If `rescheduling` is true, the call must be in the context of a + /// frame callback, and for debugging purposes the stack trace + /// stored for this callback will be the same stack trace as for the + /// current callback. + int addFrameCallback(FrameCallback callback, { bool rescheduling: false }) { _nextFrameCallbackId += 1; - _transientCallbacks[_nextFrameCallbackId] = callback; + _transientCallbacks[_nextFrameCallbackId] = new _FrameCallbackEntry(callback, rescheduling: rescheduling); return _nextFrameCallbackId; } @@ -217,11 +247,11 @@ abstract class Scheduler extends BindingBase { void _invokeTransientFrameCallbacks(Duration timeStamp) { Timeline.startSync('Animate'); assert(_debugInFrame); - Map callbacks = _transientCallbacks; - _transientCallbacks = new Map(); - callbacks.forEach((int id, FrameCallback callback) { + Map callbacks = _transientCallbacks; + _transientCallbacks = new Map(); + callbacks.forEach((int id, _FrameCallbackEntry callbackEntry) { if (!_removedIds.contains(id)) - invokeFrameCallback(callback, timeStamp); + invokeFrameCallback(callbackEntry.callback, timeStamp, callbackEntry.stack); }); _removedIds.clear(); Timeline.finishSync(); @@ -264,8 +294,12 @@ abstract class Scheduler extends BindingBase { /// Wraps the callback in a try/catch and forwards any error to /// [debugSchedulerExceptionHandler], if set. If not set, then simply prints /// the error. - void invokeFrameCallback(FrameCallback callback, Duration timeStamp) { + /// + /// Must not be called reentrantly from within a frame callback. + void invokeFrameCallback(FrameCallback callback, Duration timeStamp, [ StackTrace stack ]) { assert(callback != null); + assert(_FrameCallbackEntry.currentCallbackStack == null); + assert(() { _FrameCallbackEntry.currentCallbackStack = stack; return true; }); try { callback(timeStamp); } catch (exception, stack) { @@ -273,9 +307,55 @@ abstract class Scheduler extends BindingBase { exception: exception, stack: stack, library: 'scheduler library', - context: 'during a scheduler callback' + context: 'during a scheduler callback', + informationCollector: (stack == null) ? null : (StringBuffer information) { + information.writeln('When this callback was registered, this was the stack:\n$stack'); + } )); } + assert(() { _FrameCallbackEntry.currentCallbackStack = null; return true; }); + } + + /// Asserts that there are no registered transient callbacks; if + /// there are, prints their locations and throws an exception. + /// + /// This is expected to be called at the end of tests (the + /// flutter_test framework does it automatically in normal cases). + /// + /// To invoke this method, call it, when you expect there to be no + /// transient callbacks registered, in an assert statement with a + /// message that you want printed when a transient callback is + /// registered, as follows: + /// + /// ```dart + /// assert(Scheduler.instance.debugAssertNoTransientCallbacks( + /// 'A leak of transient callbacks was detected while doing foo.' + /// )); + /// ``` + /// + /// Does nothing if asserts are disabled. Always returns true. + bool debugAssertNoTransientCallbacks(String reason) { + assert(() { + if (transientCallbackCount > 0) { + FlutterError.reportError(new FlutterErrorDetails( + exception: reason, + library: 'scheduler library', + informationCollector: (StringBuffer information) { + information.writeln( + 'There ${ transientCallbackCount == 1 ? "was one transient callback" : "were $transientCallbackCount transient callbacks" } ' + 'left. The stack traces for when they were registered are as follows:' + ); + for (int id in _transientCallbacks.keys) { + _FrameCallbackEntry entry = _transientCallbacks[id]; + information.writeln('-- callback $id --'); + information.writeln(entry.stack); + } + } + )); + } + return true; + }); + return true; } /// Ensures that the scheduler is woken by the event loop. diff --git a/packages/flutter/lib/src/scheduler/ticker.dart b/packages/flutter/lib/src/scheduler/ticker.dart index e6d453a6e4..284f3eb9f3 100644 --- a/packages/flutter/lib/src/scheduler/ticker.dart +++ b/packages/flutter/lib/src/scheduler/ticker.dart @@ -69,12 +69,12 @@ class Ticker { // The onTick callback may have scheduled another tick already. if (isTicking && _animationId == null) - _scheduleTick(); + _scheduleTick(rescheduling: true); } - void _scheduleTick() { + void _scheduleTick({ bool rescheduling: false }) { assert(isTicking); assert(_animationId == null); - _animationId = Scheduler.instance.scheduleFrameCallback(_tick); + _animationId = Scheduler.instance.scheduleFrameCallback(_tick, rescheduling: rescheduling); } } diff --git a/packages/flutter/lib/src/services/print.dart b/packages/flutter/lib/src/services/print.dart index c96ad379c0..49f7da0633 100644 --- a/packages/flutter/lib/src/services/print.dart +++ b/packages/flutter/lib/src/services/print.dart @@ -5,6 +5,9 @@ import 'dart:async'; import 'dart:collection'; +/// Signature for [debugPrint] implementations. +typedef void DebugPrintCallback(String message, { int wrapWidth }); + /// Prints a message to the console, which you can access using the "flutter" /// tool's "logs" command ("flutter logs"). /// @@ -16,9 +19,15 @@ import 'dart:collection'; /// to this function (directly or indirectly via [debugDumpRenderTree] or /// [debugDumpApp]) and to the Dart [print] method can result in out-of-order /// messages in the logs. -void debugPrint(String message, { int wrapWidth }) { +/// +/// The implementation of this function can be replaced by setting the +/// variable to a new implementation that matches the +/// [DebugPrintCallback] signature. For example, flutter_test does this. +DebugPrintCallback debugPrint = _defaultDebugPrint; + +void _defaultDebugPrint(String message, { int wrapWidth }) { if (wrapWidth != null) { - _debugPrintBuffer.addAll(message.split('\n').expand((String line) => _wordWrap(line, wrapWidth))); + _debugPrintBuffer.addAll(message.split('\n').expand((String line) => debugWordWrap(line, wrapWidth))); } else { _debugPrintBuffer.addAll(message.split('\n')); } @@ -26,7 +35,7 @@ void debugPrint(String message, { int wrapWidth }) { _debugPrintTask(); } int _debugPrintedCharacters = 0; -int _kDebugPrintCapacity = 16 * 1024; +const int _kDebugPrintCapacity = 16 * 1024; Duration _kDebugPrintPauseTime = const Duration(seconds: 1); Queue _debugPrintBuffer = new Queue(); Stopwatch _debugPrintStopwatch = new Stopwatch(); @@ -51,10 +60,24 @@ void _debugPrintTask() { _debugPrintStopwatch.start(); } } + final RegExp _indentPattern = new RegExp('^ *(?:[-+*] |[0-9]+[.):] )?'); enum _WordWrapParseMode { inSpace, inWord, atBreak } -Iterable _wordWrap(String message, int width) sync* { - if (message.length < width) { +/// Wraps the given string at the given width. +/// +/// Wrapping occurs at space characters (U+0020). Lines that start +/// with an octothorpe ("#", U+0023) are not wrapped (so for example, +/// Dart stack traces won't be wrapped). +/// +/// This is not suitable for use with arbitrary Unicode text. For +/// example, it doesn't implement UAX #14, can't handle ideographic +/// text, doesn't hyphenate, and so forth. It is only intended for +/// formatting error messages. +/// +/// The default [debugPrint] implementation uses this for its line +/// wrapping. +Iterable debugWordWrap(String message, int width) sync* { + if (message.length < width || message[0] == '#') { yield message; return; } diff --git a/packages/flutter_test/lib/src/widget_tester.dart b/packages/flutter_test/lib/src/widget_tester.dart index 2b24355c15..f3bb48166c 100644 --- a/packages/flutter_test/lib/src/widget_tester.dart +++ b/packages/flutter_test/lib/src/widget_tester.dart @@ -80,8 +80,18 @@ class WidgetTester extends Instrumentation { super(binding: _SteppedWidgetFlutterBinding.ensureInitialized()) { timeDilation = 1.0; ui.window.onBeginFrame = null; + debugPrint = _synchronousDebugPrint; } + void _synchronousDebugPrint(String message, { int wrapWidth }) { + if (wrapWidth != null) { + print(message.split('\n').expand((String line) => debugWordWrap(line, wrapWidth)).join('\n')); + } else { + print(message); + } + } + + final FakeAsync async; final Clock clock; @@ -174,24 +184,22 @@ void testWidgets(callback(WidgetTester tester)) { callback(tester); runApp(new Container(key: new UniqueKey())); // Unmount any remaining widgets. async.flushMicrotasks(); + assert(Scheduler.instance.debugAssertNoTransientCallbacks( + 'An animation is still running even after the widget tree was disposed.' + )); assert(() { - "An animation is still running even after the widget tree was disposed."; - return Scheduler.instance.transientCallbackCount == 0; - }); - assert(() { - "A Timer is still running even after the widget tree was disposed."; + 'A Timer is still running even after the widget tree was disposed.'; return async.periodicTimerCount == 0; }); assert(() { - "A Timer is still running even after the widget tree was disposed."; + 'A Timer is still running even after the widget tree was disposed.'; return async.nonPeriodicTimerCount == 0; }); assert(async.microtaskCount == 0); // Shouldn't be possible. - assert(() { - if (tester._pendingException != null) - FlutterError.dumpErrorToConsole(tester._pendingException); - return tester._pendingException == null; - }); + if (tester._pendingException != null) { + FlutterError.dumpErrorToConsole(tester._pendingException); + throw 'An exception (shown above) was thrown during the test.'; + } } finally { FlutterError.onError = oldHandler; } diff --git a/travis/test.sh b/travis/test.sh index e36f2fe5fa..4043ec54b4 100755 --- a/travis/test.sh +++ b/travis/test.sh @@ -14,4 +14,8 @@ flutter analyze --flutter-repo --no-current-directory --no-current-package --con (cd packages/flx; dart -c test/all.dart) (cd packages/newton; dart -c test/all.dart) +(cd dev/manual_tests; flutter test) +(cd examples/hello_world; flutter test) +(cd examples/layers; flutter test) +(cd examples/material_gallery; flutter test) (cd examples/stocks; flutter test)