Skip to content

Commit 7861d02

Browse files
committed
Fix dependency skew. (flutter#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.
1 parent 74fe401 commit 7861d02

File tree

14 files changed

+330
-31
lines changed

14 files changed

+330
-31
lines changed

dev/manual_tests/pubspec.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,7 @@ name: flutter_manual_tests
22
dependencies:
33
flutter:
44
path: ../../packages/flutter
5+
dev_dependencies:
6+
test: any # flutter_test provides the version constraints
7+
flutter_test:
8+
path: ../../packages/flutter_test
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// Copyright 2016 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:flutter/material.dart';
6+
import 'package:flutter_test/flutter_test.dart';
7+
import 'package:test/test.dart';
8+
9+
import '../card_collection.dart' as card_collection;
10+
11+
void main() {
12+
test("Card Collection smoke test", () {
13+
testWidgets((WidgetTester tester) {
14+
card_collection.main(); // builds the app and schedules a frame but doesn't trigger one
15+
tester.pump(); // see https://github.com/flutter/flutter/issues/1865
16+
tester.pump(); // triggers a frame
17+
18+
Element navigationMenu = tester.findElement((Element element) {
19+
Widget widget = element.widget;
20+
if (widget is Tooltip)
21+
return widget.message == 'Open navigation menu';
22+
return false;
23+
});
24+
25+
expect(navigationMenu, isNotNull);
26+
27+
tester.tap(navigationMenu);
28+
tester.pump(); // start opening menu
29+
tester.pump(const Duration(seconds: 1)); // wait til it's really opened
30+
31+
// smoke test for various checkboxes
32+
tester.tap(tester.findText('Make card labels editable'));
33+
tester.pump();
34+
tester.tap(tester.findText('Let the sun shine'));
35+
tester.pump();
36+
tester.tap(tester.findText('Make card labels editable'));
37+
tester.pump();
38+
tester.tap(tester.findText('Vary font sizes'));
39+
tester.pump();
40+
});
41+
});
42+
}

examples/hello_world/pubspec.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,7 @@ name: hello_world
22
dependencies:
33
flutter:
44
path: ../../packages/flutter
5+
dev_dependencies:
6+
test: any # flutter_test provides the version constraints
7+
flutter_test:
8+
path: ../../packages/flutter_test
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright 2016 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:flutter_test/flutter_test.dart';
6+
import 'package:test/test.dart';
7+
8+
import '../lib/main.dart' as hello_world;
9+
10+
void main() {
11+
test("Hello world smoke test", () {
12+
testWidgets((WidgetTester tester) {
13+
hello_world.main(); // builds the app and schedules a frame but doesn't trigger one
14+
tester.pump(); // triggers a frame
15+
16+
expect(tester.findText('Hello, world!'), isNotNull);
17+
});
18+
});
19+
}

examples/layers/pubspec.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,7 @@ name: flutter_examples_layers
22
dependencies:
33
flutter:
44
path: ../../packages/flutter
5+
dev_dependencies:
6+
test: any # flutter_test provides the version constraints
7+
flutter_test:
8+
path: ../../packages/flutter_test
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright 2015 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import '../rendering/src/sector_layout.dart';
6+
import 'package:test/test.dart';
7+
8+
void main() {
9+
test('SectorConstraints', () {
10+
expect(const SectorConstraints().isTight, isFalse);
11+
});
12+
}

examples/material_gallery/pubspec.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,8 @@ dependencies:
1111
flutter_markdown:
1212
path: ../../packages/flutter_markdown
1313
flutter_gallery_assets: '0.0.15'
14+
15+
dev_dependencies:
16+
test: any # flutter_test provides the version constraints
17+
flutter_test:
18+
path: ../../packages/flutter_test
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
// Copyright 2016 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:flutter/material.dart';
6+
import 'package:flutter_test/flutter_test.dart';
7+
import 'package:test/test.dart';
8+
9+
import '../lib/main.dart' as material_gallery;
10+
11+
void main() {
12+
test('Material Gallery app smoke test', () {
13+
testWidgets((WidgetTester tester) {
14+
material_gallery.main(); // builds the app and schedules a frame but doesn't trigger one
15+
tester.pump(); // see https://github.com/flutter/flutter/issues/1865
16+
tester.pump(); // triggers a frame
17+
18+
// Try loading Weather demo
19+
tester.tap(tester.findText('Demos'));
20+
tester.pump();
21+
tester.pump(const Duration(seconds: 1)); // wait til it's really opened
22+
23+
tester.tap(tester.findText('Weather'));
24+
tester.pump();
25+
tester.pump(const Duration(seconds: 1)); // wait til it's really opened
26+
27+
// Go back
28+
Element backButton = tester.findElement((Element element) {
29+
Widget widget = element.widget;
30+
if (widget is Tooltip)
31+
return widget.message == 'Back';
32+
return false;
33+
});
34+
expect(backButton, isNotNull);
35+
tester.tap(backButton);
36+
tester.pump(); // start going back
37+
tester.pump(const Duration(seconds: 1)); // wait til it's finished
38+
39+
// Open menu
40+
Element navigationMenu = tester.findElement((Element element) {
41+
Widget widget = element.widget;
42+
if (widget is Tooltip)
43+
return widget.message == 'Open navigation menu';
44+
return false;
45+
});
46+
expect(navigationMenu, isNotNull);
47+
tester.tap(navigationMenu);
48+
tester.pump(); // start opening menu
49+
tester.pump(const Duration(seconds: 1)); // wait til it's really opened
50+
51+
// switch theme
52+
tester.tap(tester.findText('Dark'));
53+
tester.pump();
54+
tester.pump(const Duration(seconds: 1)); // wait til it's changed
55+
56+
// switch theme
57+
tester.tap(tester.findText('Light'));
58+
tester.pump();
59+
tester.pump(const Duration(seconds: 1)); // wait til it's changed
60+
});
61+
});
62+
}

packages/flutter/lib/src/material/toggleable.dart

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,38 @@ abstract class RenderToggleable extends RenderConstrainedBox implements Semantic
117117
TapGestureRecognizer _tap;
118118
Point _downPosition;
119119

120+
@override
121+
void attach(PipelineOwner owner) {
122+
super.attach(owner);
123+
if (_positionController != null) {
124+
if (value)
125+
_positionController.forward();
126+
else
127+
_positionController.reverse();
128+
}
129+
if (_reactionController != null && isInteractive) {
130+
switch (_reactionController.status) {
131+
case AnimationStatus.forward:
132+
_reactionController.forward();
133+
break;
134+
case AnimationStatus.reverse:
135+
_reactionController.reverse();
136+
break;
137+
case AnimationStatus.dismissed:
138+
case AnimationStatus.completed:
139+
// nothing to do
140+
break;
141+
}
142+
}
143+
}
144+
145+
@override
146+
void detach() {
147+
_positionController?.stop();
148+
_reactionController?.stop();
149+
super.detach();
150+
}
151+
120152
void _handlePositionStateChanged(AnimationStatus status) {
121153
if (isInteractive) {
122154
if (status == AnimationStatus.completed && !_value)

packages/flutter/lib/src/scheduler/scheduler.dart

Lines changed: 92 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,26 @@ typedef bool SchedulingStrategy({ int priority, Scheduler scheduler });
3030
///
3131
/// Combines the task and its priority.
3232
class _TaskEntry {
33+
const _TaskEntry(this.task, this.priority);
3334
final VoidCallback task;
3435
final int priority;
36+
}
3537

36-
const _TaskEntry(this.task, this.priority);
38+
class _FrameCallbackEntry {
39+
_FrameCallbackEntry(this.callback, { bool rescheduling: false }) {
40+
assert(() {
41+
if (rescheduling) {
42+
assert(currentCallbackStack != null);
43+
stack = currentCallbackStack;
44+
} else {
45+
stack = StackTrace.current;
46+
}
47+
return true;
48+
});
49+
}
50+
static StackTrace currentCallbackStack;
51+
final FrameCallback callback;
52+
StackTrace stack;
3753
}
3854

3955
class Priority {
@@ -141,7 +157,7 @@ abstract class Scheduler extends BindingBase {
141157
}
142158

143159
int _nextFrameCallbackId = 0; // positive
144-
Map<int, FrameCallback> _transientCallbacks = <int, FrameCallback>{};
160+
Map<int, _FrameCallbackEntry> _transientCallbacks = <int, _FrameCallbackEntry>{};
145161
final Set<int> _removedIds = new HashSet<int>();
146162

147163
int get transientCallbackCount => _transientCallbacks.length;
@@ -150,9 +166,14 @@ abstract class Scheduler extends BindingBase {
150166
///
151167
/// Adds the given callback to the list of frame-callbacks and ensures that a
152168
/// frame is scheduled.
153-
int scheduleFrameCallback(FrameCallback callback) {
169+
///
170+
/// If `rescheduling` is true, the call must be in the context of a
171+
/// frame callback, and for debugging purposes the stack trace
172+
/// stored for this callback will be the same stack trace as for the
173+
/// current callback.
174+
int scheduleFrameCallback(FrameCallback callback, { bool rescheduling: false }) {
154175
_ensureBeginFrameCallback();
155-
return addFrameCallback(callback);
176+
return addFrameCallback(callback, rescheduling: rescheduling);
156177
}
157178

158179
/// Adds a frame callback.
@@ -162,9 +183,18 @@ abstract class Scheduler extends BindingBase {
162183
///
163184
/// The registered callbacks are executed in the order in which they have been
164185
/// registered.
165-
int addFrameCallback(FrameCallback callback) {
186+
///
187+
/// Callbacks registered with this method will not be invoked until
188+
/// a frame is requested. To register a callback and ensure that a
189+
/// frame is immediately scheduled, use [scheduleFrameCallback].
190+
///
191+
/// If `rescheduling` is true, the call must be in the context of a
192+
/// frame callback, and for debugging purposes the stack trace
193+
/// stored for this callback will be the same stack trace as for the
194+
/// current callback.
195+
int addFrameCallback(FrameCallback callback, { bool rescheduling: false }) {
166196
_nextFrameCallbackId += 1;
167-
_transientCallbacks[_nextFrameCallbackId] = callback;
197+
_transientCallbacks[_nextFrameCallbackId] = new _FrameCallbackEntry(callback, rescheduling: rescheduling);
168198
return _nextFrameCallbackId;
169199
}
170200

@@ -217,11 +247,11 @@ abstract class Scheduler extends BindingBase {
217247
void _invokeTransientFrameCallbacks(Duration timeStamp) {
218248
Timeline.startSync('Animate');
219249
assert(_debugInFrame);
220-
Map<int, FrameCallback> callbacks = _transientCallbacks;
221-
_transientCallbacks = new Map<int, FrameCallback>();
222-
callbacks.forEach((int id, FrameCallback callback) {
250+
Map<int, _FrameCallbackEntry> callbacks = _transientCallbacks;
251+
_transientCallbacks = new Map<int, _FrameCallbackEntry>();
252+
callbacks.forEach((int id, _FrameCallbackEntry callbackEntry) {
223253
if (!_removedIds.contains(id))
224-
invokeFrameCallback(callback, timeStamp);
254+
invokeFrameCallback(callbackEntry.callback, timeStamp, callbackEntry.stack);
225255
});
226256
_removedIds.clear();
227257
Timeline.finishSync();
@@ -264,18 +294,68 @@ abstract class Scheduler extends BindingBase {
264294
/// Wraps the callback in a try/catch and forwards any error to
265295
/// [debugSchedulerExceptionHandler], if set. If not set, then simply prints
266296
/// the error.
267-
void invokeFrameCallback(FrameCallback callback, Duration timeStamp) {
297+
///
298+
/// Must not be called reentrantly from within a frame callback.
299+
void invokeFrameCallback(FrameCallback callback, Duration timeStamp, [ StackTrace stack ]) {
268300
assert(callback != null);
301+
assert(_FrameCallbackEntry.currentCallbackStack == null);
302+
assert(() { _FrameCallbackEntry.currentCallbackStack = stack; return true; });
269303
try {
270304
callback(timeStamp);
271305
} catch (exception, stack) {
272306
FlutterError.reportError(new FlutterErrorDetails(
273307
exception: exception,
274308
stack: stack,
275309
library: 'scheduler library',
276-
context: 'during a scheduler callback'
310+
context: 'during a scheduler callback',
311+
informationCollector: (stack == null) ? null : (StringBuffer information) {
312+
information.writeln('When this callback was registered, this was the stack:\n$stack');
313+
}
277314
));
278315
}
316+
assert(() { _FrameCallbackEntry.currentCallbackStack = null; return true; });
317+
}
318+
319+
/// Asserts that there are no registered transient callbacks; if
320+
/// there are, prints their locations and throws an exception.
321+
///
322+
/// This is expected to be called at the end of tests (the
323+
/// flutter_test framework does it automatically in normal cases).
324+
///
325+
/// To invoke this method, call it, when you expect there to be no
326+
/// transient callbacks registered, in an assert statement with a
327+
/// message that you want printed when a transient callback is
328+
/// registered, as follows:
329+
///
330+
/// ```dart
331+
/// assert(Scheduler.instance.debugAssertNoTransientCallbacks(
332+
/// 'A leak of transient callbacks was detected while doing foo.'
333+
/// ));
334+
/// ```
335+
///
336+
/// Does nothing if asserts are disabled. Always returns true.
337+
bool debugAssertNoTransientCallbacks(String reason) {
338+
assert(() {
339+
if (transientCallbackCount > 0) {
340+
FlutterError.reportError(new FlutterErrorDetails(
341+
exception: reason,
342+
library: 'scheduler library',
343+
informationCollector: (StringBuffer information) {
344+
information.writeln(
345+
'There ${ transientCallbackCount == 1 ? "was one transient callback" : "were $transientCallbackCount transient callbacks" } '
346+
'left. The stack traces for when they were registered are as follows:'
347+
);
348+
for (int id in _transientCallbacks.keys) {
349+
_FrameCallbackEntry entry = _transientCallbacks[id];
350+
information.writeln('-- callback $id --');
351+
information.writeln(entry.stack);
352+
}
353+
}
354+
));
355+
}
356+
return true;
357+
});
358+
return true;
279359
}
280360

281361
/// Ensures that the scheduler is woken by the event loop.

0 commit comments

Comments
 (0)