From b2a27c109c0c87febb0239dcf74d99eb6842e8f7 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 31 Jan 2020 09:22:30 -0800 Subject: [PATCH] Be clearer about when and why we override HttpClient in tests (#49844) --- dev/bots/test.dart | 24 +++++++++++++++++ .../flutter_test/lib/src/_binding_io.dart | 16 ++++++++++++ packages/flutter_test/lib/src/binding.dart | 7 +++++ .../test/bindings_test_failure.dart | 26 +++++++++++++++++++ 4 files changed, 73 insertions(+) create mode 100644 packages/flutter_test/test/bindings_test_failure.dart diff --git a/dev/bots/test.dart b/dev/bots/test.dart index dd9cba084c..315c2346e8 100644 --- a/dev/bots/test.dart +++ b/dev/bots/test.dart @@ -466,6 +466,30 @@ Future _runFrameworkTests() async { 'FLUTTER_EXPERIMENTAL_BUILD': 'true', }, ); + const String httpClientWarning = + 'Warning: At least one test in this suite creates an HttpClient. When\n' + 'running a test suite that uses TestWidgetsFlutterBinding, all HTTP\n' + 'requests will return status code 400, and no network request will\n' + 'actually be made. Any test expecting an real network connection and\n' + 'status code will fail.\n' + 'To test code that needs an HttpClient, provide your own HttpClient\n' + 'implementation to the code under test, so that your test can\n' + 'consistently provide a testable response to the code under test.'; + await _runFlutterTest( + path.join(flutterRoot, 'packages', 'flutter_test'), + script: path.join('test', 'bindings_test_failure.dart'), + expectFailure: true, + printOutput: false, + outputChecker: (CapturedOutput output) { + final Iterable matches = httpClientWarning.allMatches(output.stdout); + if (matches == null || matches.isEmpty || matches.length > 1) { + return 'Failed to print warning about HttpClientUsage, or printed it too many times.\n' + 'stdout:\n${output.stdout}'; + } + return null; + }, + tableData: bigqueryApi?.tabledata, + ); } await selectSubshard({ diff --git a/packages/flutter_test/lib/src/_binding_io.dart b/packages/flutter_test/lib/src/_binding_io.dart index 188e4dfa91..40d38cace6 100644 --- a/packages/flutter_test/lib/src/_binding_io.dart +++ b/packages/flutter_test/lib/src/_binding_io.dart @@ -11,6 +11,9 @@ import 'package:flutter/services.dart'; import 'package:flutter/widgets.dart'; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; +// ignore: deprecated_member_use +import 'package:test_api/test_api.dart' as test_package; + import 'binding.dart'; @@ -73,8 +76,21 @@ void mockFlutterAssets() { /// If another [HttpClient] is provided using [HttpOverrides.runZoned], that will /// take precedence over this provider. class _MockHttpOverrides extends HttpOverrides { + bool warningPrinted = false; @override HttpClient createHttpClient(SecurityContext _) { + if (!warningPrinted) { + test_package.printOnFailure( + 'Warning: At least one test in this suite creates an HttpClient. When\n' + 'running a test suite that uses TestWidgetsFlutterBinding, all HTTP\n' + 'requests will return status code 400, and no network request will\n' + 'actually be made. Any test expecting an real network connection and\n' + 'status code will fail.\n' + 'To test code that needs an HttpClient, provide your own HttpClient\n' + 'implementation to the code under test, so that your test can\n' + 'consistently provide a testable response to the code under test.'); + warningPrinted = true; + } return _MockHttpClient(); } } diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index fbdfab562c..ec79f32982 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -147,6 +147,13 @@ class TestDefaultBinaryMessenger extends BinaryMessenger { /// /// When using these bindings, certain features are disabled. For /// example, [timeDilation] is reset to 1.0 on initialization. +/// +/// In non-browser tests, the binding overrides `HttpClient` creation with a +/// fake client that always returns a status code of 400. This is to prevent +/// tests from making network calls, which could introduce flakiness. A test +/// that actually needs to make a network call should provide its own +/// `HttpClient` to the code making the call, so that it can appropriately mock +/// or fake responses. abstract class TestWidgetsFlutterBinding extends BindingBase with ServicesBinding, SchedulerBinding, diff --git a/packages/flutter_test/test/bindings_test_failure.dart b/packages/flutter_test/test/bindings_test_failure.dart new file mode 100644 index 0000000000..4d5866b01f --- /dev/null +++ b/packages/flutter_test/test/bindings_test_failure.dart @@ -0,0 +1,26 @@ +// Copyright 2014 The Flutter 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'; + +import 'package:flutter_test/flutter_test.dart'; + +// This test checks that we output something on test failure where the test +// creates an HttpClient. +// It should not be run as part of a normal test suite, as it is expected to +// fail. See dev/bots/test.dart, which runs this test, checks that it fails, +// and prints the expected warning about HttpClient usage. +// We don't run this for browser tests, since we don't override the behavior +// in browser tests. + +void main() { + test('Http Warning Message', () { + TestWidgetsFlutterBinding.ensureInitialized(); + HttpClient(); + // Make sure we only add the message once. + HttpClient(); + HttpClient(); + fail('Intentional'); + }); +}