From 135864f4a4023dbb25e8c4b287d09cb12f6c92c9 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 28 Aug 2024 14:12:12 -0700 Subject: [PATCH] Migrate`header_guard_check` to `package:test`. (#54811) Work towards https://github.com/flutter/flutter/issues/133569. I also augmented the `run_tests.py` script to support an incremental migration to `package:test`, PTAL. --- testing/run_tests.py | 57 +++++++++++-- tools/header_guard_check/pubspec.yaml | 5 +- .../test/header_file_test.dart | 85 +++++++++++-------- .../test/header_guard_check_test.dart | 6 +- 4 files changed, 102 insertions(+), 51 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 60ed51043e793..24bab000475b9 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') @@ -65,8 +69,9 @@ 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, expect_failure: bool = False, env: typing.Dict[str, str] = None, @@ -81,12 +86,13 @@ 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() process = subprocess.Popen( cmd, + cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, env=env, @@ -905,14 +911,49 @@ 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, '%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( 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 file: + # Check if either "dependencies" or "dev_dependencies" contains "test". + data = yaml.safe_load(file) + if data is None: + return False + deps = data.get('dependencies', {}) + if 'test' in deps: + return True + dev_deps = data.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');