From 36ab690c133b7d2aa479fec601a1ad375df4a5b8 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 27 Aug 2024 13:34:25 -0700 Subject: [PATCH 1/7] Add support for package:test in the engine test runner, and use it in header_guard_check. --- testing/run_tests.py | 45 ++++++++-- tools/header_guard_check/pubspec.yaml | 5 +- .../test/header_file_test.dart | 85 +++++++++++-------- .../test/header_guard_check_test.dart | 6 +- 4 files changed, 92 insertions(+), 49 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 60ed51043e793..7021acfa29c05 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -23,12 +23,16 @@ # Explicitly import the parts of sys that are needed. This is to avoid using # sys.stdout and sys.stderr directly. Instead, only the logger defined below # should be used for output. -from sys import exit as sys_exit, platform as sys_platform +from sys import exit as sys_exit, platform as sys_platform, path as sys_path import tempfile import time import typing import xvfb +THIS_DIR = os.path.abspath(os.path.dirname(__file__)) +sys_path.insert(0, os.path.join(THIS_DIR, '..', 'third_party', 'pyyaml', 'lib')) +import yaml # pylint: disable=import-error, wrong-import-position + SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) BUILDROOT_DIR = os.path.abspath(os.path.join(os.path.realpath(__file__), '..', '..', '..')) OUT_DIR = os.path.join(BUILDROOT_DIR, 'out') @@ -905,15 +909,44 @@ def gather_dart_smoke_test(build_dir, test_filter): def gather_dart_package_tests(build_dir, package_path, extra_opts): - dart_tests = glob.glob('%s/test/*_test.dart' % package_path) - if not dart_tests: - raise Exception('No tests found for Dart package at %s' % package_path) - for dart_test_file in dart_tests: - opts = ['--disable-dart-dev', dart_test_file] + extra_opts + if uses_package_test_runner(package_path): + # Package that use package:test (dart test) do not support command-line arguments. + # + # The usual workaround is to use Platform.environment, but that would require changing + # the test execution a tiny bit. TODO(https://github.com/flutter/flutter/issues/133569). + # + # Until then, assert that no extra_opts are passed and explain the limitation. + assert not extra_opts, 'Package %s uses package:test and does not support command-line arguments' % package_path + opts = ['--disable-dart-dev', 'test'] yield EngineExecutableTask( build_dir, os.path.join('dart-sdk', 'bin', 'dart'), None, flags=opts, cwd=package_path ) + else: + dart_tests = glob.glob('%s/test/*_test.dart' % package_path) + if not dart_tests: + raise Exception('No tests found for Dart package at %s' % package_path) + for dart_test_file in dart_tests: + opts = ['--disable-dart-dev', dart_test_file] + extra_opts + yield EngineExecutableTask( + build_dir, os.path.join('dart-sdk', 'bin', 'dart'), None, flags=opts, cwd=package_path + ) +# Returns whether the given package path should be tested with `dart test`. +# +# Inferred by a dependency on the `package:test` package in the pubspec.yaml. +def uses_package_test_runner(package): + pubspec = os.path.join(package, 'pubspec.yaml') + if not os.path.exists(pubspec): + return False + with open(pubspec, 'r') as f: + # Check if either "dependencies" or "dev_dependencies" contains "test". + deps = yaml.safe_load(f).get('dependencies', {}) + if 'test' in deps: + return True + dev_deps = yaml.safe_load(f).get('dev_dependencies', {}) + if 'test' in dev_deps: + return True + return False # Returns a list of Dart packages to test. # diff --git a/tools/header_guard_check/pubspec.yaml b/tools/header_guard_check/pubspec.yaml index 766ef84e9aba8..7401a16826bf4 100644 --- a/tools/header_guard_check/pubspec.yaml +++ b/tools/header_guard_check/pubspec.yaml @@ -21,8 +21,5 @@ dependencies: source_span: any dev_dependencies: - async_helper: any - expect: any - litetest: any process_fakes: any - smith: any + test: any diff --git a/tools/header_guard_check/test/header_file_test.dart b/tools/header_guard_check/test/header_file_test.dart index a61d0edaca0d7..833ef9b9bb3e0 100644 --- a/tools/header_guard_check/test/header_file_test.dart +++ b/tools/header_guard_check/test/header_file_test.dart @@ -1,18 +1,21 @@ // Copyright 2013 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. +@TestOn('vm') +library; import 'dart:io' as io; import 'package:header_guard_check/src/header_file.dart'; -import 'package:litetest/litetest.dart'; import 'package:path/path.dart' as p; import 'package:source_span/source_span.dart'; +import 'package:test/test.dart'; -Future main(List args) async { +Future main() async { void withTestFile(String path, String contents, void Function(io.File) fn) { // Create a temporary file and delete it when we're done. - final io.Directory tempDir = io.Directory.systemTemp.createTempSync('header_guard_check_test'); + final io.Directory tempDir = + io.Directory.systemTemp.createTempSync('header_guard_check_test'); final io.File file = io.File(p.join(tempDir.path, path)); file.writeAsStringSync(contents); try { @@ -157,7 +160,8 @@ Future main(List args) async { guard: null, pragmaOnce: null, ); - expect(headerFile.computeExpectedName(engineRoot: ''), endsWith('FOO_BAR_BAZ_H_')); + expect(headerFile.computeExpectedName(engineRoot: ''), + endsWith('FOO_BAR_BAZ_H_')); } }); @@ -240,14 +244,16 @@ Future main(List args) async { withTestFile('foo.h', input, (io.File file) { final HeaderFile headerFile = HeaderFile.parse(file.path); expect(headerFile.fix(engineRoot: p.dirname(file.path)), isTrue); - expect(file.readAsStringSync(), [ - '#ifndef FLUTTER_FOO_H_', - '#define FLUTTER_FOO_H_', - '', - '// ...', - '#endif // FLUTTER_FOO_H_', - '', - ].join('\n')); + expect( + file.readAsStringSync(), + [ + '#ifndef FLUTTER_FOO_H_', + '#define FLUTTER_FOO_H_', + '', + '// ...', + '#endif // FLUTTER_FOO_H_', + '', + ].join('\n')); }); }); @@ -261,13 +267,15 @@ Future main(List args) async { withTestFile('foo.h', input, (io.File file) { final HeaderFile headerFile = HeaderFile.parse(file.path); expect(headerFile.fix(engineRoot: p.dirname(file.path)), isTrue); - expect(file.readAsStringSync(), [ - '#ifndef FLUTTER_FOO_H_', - '#define FLUTTER_FOO_H_', - '', - '#endif // FLUTTER_FOO_H_', - '', - ].join('\n')); + expect( + file.readAsStringSync(), + [ + '#ifndef FLUTTER_FOO_H_', + '#define FLUTTER_FOO_H_', + '', + '#endif // FLUTTER_FOO_H_', + '', + ].join('\n')); }); }); @@ -287,27 +295,30 @@ Future main(List args) async { withTestFile('foo.h', input, (io.File file) { final HeaderFile headerFile = HeaderFile.parse(file.path); expect(headerFile.fix(engineRoot: p.dirname(file.path)), isTrue); - expect(file.readAsStringSync(), [ - '// 1.', - '// 2.', - '// 3.', - '', - '#ifndef FLUTTER_FOO_H_', - '#define FLUTTER_FOO_H_', - '', - "#import 'flutter/shell/platform/darwin/Flutter.h'", - '', - '@protocl Flutter', - '', - '@end', - '', - '#endif // FLUTTER_FOO_H_', - '', - ].join('\n')); + expect( + file.readAsStringSync(), + [ + '// 1.', + '// 2.', + '// 3.', + '', + '#ifndef FLUTTER_FOO_H_', + '#define FLUTTER_FOO_H_', + '', + "#import 'flutter/shell/platform/darwin/Flutter.h'", + '', + '@protocl Flutter', + '', + '@end', + '', + '#endif // FLUTTER_FOO_H_', + '', + ].join('\n')); }); }); - test('does not touch a file with an existing guard and another #define', () { + test('does not touch a file with an existing guard and another #define', + () { final String input = [ '// 1.', '// 2.', diff --git a/tools/header_guard_check/test/header_guard_check_test.dart b/tools/header_guard_check/test/header_guard_check_test.dart index cf4ed35435b4f..b442ca431ed2d 100644 --- a/tools/header_guard_check/test/header_guard_check_test.dart +++ b/tools/header_guard_check/test/header_guard_check_test.dart @@ -1,15 +1,17 @@ // Copyright 2013 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. +@TestOn('vm') +library; import 'dart:io' as io; import 'package:engine_repo_tools/engine_repo_tools.dart'; import 'package:header_guard_check/header_guard_check.dart'; -import 'package:litetest/litetest.dart'; import 'package:path/path.dart' as p; +import 'package:test/test.dart'; -Future main(List args) async { +Future main() async { void withTestRepository(String path, void Function(io.Directory) fn) { // Create a temporary directory and delete it when we're done. final io.Directory tempDir = io.Directory.systemTemp.createTempSync('header_guard_check_test'); From 08e106d13caa85c4086b9376c9ad1b222d28d44d Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 27 Aug 2024 13:36:24 -0700 Subject: [PATCH 2/7] ++ --- testing/run_tests.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/testing/run_tests.py b/testing/run_tests.py index 7021acfa29c05..68a8a74a4a8c7 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -931,6 +931,7 @@ def gather_dart_package_tests(build_dir, package_path, extra_opts): build_dir, os.path.join('dart-sdk', 'bin', 'dart'), None, flags=opts, cwd=package_path ) + # Returns whether the given package path should be tested with `dart test`. # # Inferred by a dependency on the `package:test` package in the pubspec.yaml. @@ -948,6 +949,7 @@ def uses_package_test_runner(package): return True return False + # Returns a list of Dart packages to test. # # The first element of each tuple in the returned list is the path to the Dart From 75781edccf528ba2a5d87dc9a3bd313f9dfe9c1f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 27 Aug 2024 14:09:49 -0700 Subject: [PATCH 3/7] Add None guard. --- testing/run_tests.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 68a8a74a4a8c7..9bcfdde1682b8 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -941,10 +941,13 @@ def uses_package_test_runner(package): return False with open(pubspec, 'r') as f: # Check if either "dependencies" or "dev_dependencies" contains "test". - deps = yaml.safe_load(f).get('dependencies', {}) + data = yaml.safe_load(f) + if data is None: + return False + deps = data.get('dependencies', {}) if 'test' in deps: return True - dev_deps = yaml.safe_load(f).get('dev_dependencies', {}) + dev_deps = data.get('dev_dependencies', {}) if 'test' in dev_deps: return True return False From c90790c3463e27261ca5d25c9e79bd6a9ce68d1d Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 27 Aug 2024 17:08:07 -0700 Subject: [PATCH 4/7] ++ --- testing/run_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 9bcfdde1682b8..9b27d1b9169fa 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -917,7 +917,7 @@ def gather_dart_package_tests(build_dir, package_path, extra_opts): # # Until then, assert that no extra_opts are passed and explain the limitation. assert not extra_opts, 'Package %s uses package:test and does not support command-line arguments' % package_path - opts = ['--disable-dart-dev', 'test'] + opts = ['test', '--disable-dart-dev'] yield EngineExecutableTask( build_dir, os.path.join('dart-sdk', 'bin', 'dart'), None, flags=opts, cwd=package_path ) From d38616f3f6067a820b320eecefe61680e5b2cbab Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 28 Aug 2024 09:18:32 -0700 Subject: [PATCH 5/7] Remove --disable-dart-dev for now. --- testing/run_tests.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 9b27d1b9169fa..17dfff426923d 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -71,6 +71,7 @@ def is_asan(build_dir): def run_cmd( cmd: typing.List[str], + cwd: str = None, forbidden_output: typing.List[str] = None, expect_failure: bool = False, env: typing.Dict[str, str] = None, @@ -85,7 +86,7 @@ def run_cmd( command_string = ' '.join(cmd) print_divider('>') - logger.info('Running command "%s"', command_string) + logger.info('Running command "%s" in "%s"', command_string, cwd) start_time = time.time() @@ -917,7 +918,8 @@ def gather_dart_package_tests(build_dir, package_path, extra_opts): # # Until then, assert that no extra_opts are passed and explain the limitation. assert not extra_opts, 'Package %s uses package:test and does not support command-line arguments' % package_path - opts = ['test', '--disable-dart-dev'] + # TODO(https://github.com/flutter/flutter/issues/154263): Restore `--disable-dart-dev`. + opts = ['test'] yield EngineExecutableTask( build_dir, os.path.join('dart-sdk', 'bin', 'dart'), None, flags=opts, cwd=package_path ) From 83bec274a029d7f0e5e134924fe41ce38bc3411b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 28 Aug 2024 10:33:02 -0700 Subject: [PATCH 6/7] ++ --- testing/run_tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/run_tests.py b/testing/run_tests.py index 17dfff426923d..50af543702435 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -92,6 +92,7 @@ def run_cmd( process = subprocess.Popen( cmd, + cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, env=env, From 4299c713691d71a1d8aad7a0d623b867671ed845 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 28 Aug 2024 11:29:18 -0700 Subject: [PATCH 7/7] ++ --- testing/run_tests.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 50af543702435..24bab000475b9 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -69,7 +69,7 @@ def is_asan(build_dir): return False -def run_cmd( +def run_cmd( # pylint: disable=too-many-arguments cmd: typing.List[str], cwd: str = None, forbidden_output: typing.List[str] = None, @@ -918,7 +918,7 @@ def gather_dart_package_tests(build_dir, package_path, extra_opts): # the test execution a tiny bit. TODO(https://github.com/flutter/flutter/issues/133569). # # Until then, assert that no extra_opts are passed and explain the limitation. - assert not extra_opts, 'Package %s uses package:test and does not support command-line arguments' % package_path + assert not extra_opts, '%s uses package:test and cannot use CLI args' % package_path # TODO(https://github.com/flutter/flutter/issues/154263): Restore `--disable-dart-dev`. opts = ['test'] yield EngineExecutableTask( @@ -942,9 +942,9 @@ def uses_package_test_runner(package): pubspec = os.path.join(package, 'pubspec.yaml') if not os.path.exists(pubspec): return False - with open(pubspec, 'r') as f: + with open(pubspec, 'r') as file: # Check if either "dependencies" or "dev_dependencies" contains "test". - data = yaml.safe_load(f) + data = yaml.safe_load(file) if data is None: return False deps = data.get('dependencies', {})