diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 28f95544..7f4ebd08 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,14 @@ jobs: - name: Install workspace dependencies run: dart pub get + # Integration target selection treats an unchanged lockfile as unchanged resolution. + - name: Check committed lockfile + run: | + if ! git diff --exit-code pubspec.lock; then + echo "::error file=pubspec.lock::dart pub get changed pubspec.lock. Run dart pub get and commit pubspec.lock." + exit 1 + fi + - name: Set PR diff base if: github.event_name == 'pull_request' run: | diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index 473c447a..90593374 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -4,13 +4,17 @@ on: pull_request: push: branches: [main] + schedule: + # Weekly full regression run; pushes to main only run affected targets. + - cron: '23 19 * * 0' workflow_dispatch: permissions: contents: read concurrency: - group: integration-${{ github.event.pull_request.number || github.ref }} + # Only PR runs share a group: a replaced pending run on main would leave its pushed changes untested. + group: integration-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: @@ -30,13 +34,16 @@ jobs: id: targets env: EVENT: ${{ github.event_name }} - BASE: ${{ github.event.pull_request.base.sha }} - HEAD: ${{ github.event.pull_request.head.sha }} + PR_BASE: ${{ github.event.pull_request.base.sha }} + PR_HEAD: ${{ github.event.pull_request.head.sha }} + PUSH_BEFORE: ${{ github.event.before }} run: | - args=() - if [[ "$EVENT" == "pull_request" ]]; then - args+=(--base "$BASE" --head "$HEAD") - fi + case "$EVENT" in + pull_request) args=(--base "$PR_BASE" --head "$PR_HEAD") ;; + push) args=(--base "$PUSH_BEFORE" --head "$GITHUB_SHA") ;; + # schedule and workflow_dispatch run every target. + *) args=() ;; + esac python3 -B tool/ci/select_integration_targets.py "${args[@]}" --output "$GITHUB_OUTPUT" integration: diff --git a/tool/ci/select_integration_targets.py b/tool/ci/select_integration_targets.py index 27142f2e..39694a00 100644 --- a/tool/ci/select_integration_targets.py +++ b/tool/ci/select_integration_targets.py @@ -5,6 +5,7 @@ from pathlib import Path import re import subprocess +import sys ROOT = Path(__file__).resolve().parents[2] @@ -61,6 +62,16 @@ def load_targets(root): return targets +def diffable_base(root, base): + """Return base when it names a local commit, or None when a push diff cannot be computed. + + A new branch reports an all-zero base, and a force push can report a base + that is no longer in the history. + """ + exists = subprocess.run(['git', 'cat-file', '-e', f'{base}^{{commit}}'], cwd=root, capture_output=True) + return base if exists.returncode == 0 else None + + def changed_files(root, base, head): # Include both sides of moves so moving a file out of a package still tests it. output = subprocess.check_output( @@ -70,29 +81,94 @@ def changed_files(root, base, head): return output.decode().split('\0')[:-1] -def select_targets(targets, paths): +def read_pubspec(text): + """Read the package name, workspace members, and direct dependency names from pubspec text. + + Only the top-level keys this selector needs are parsed, line by line, because + the standard library has no YAML parser. + """ + pubspec = {'name': None, 'workspace': [], 'dependencies': set()} + section = child_indent = None + for line in text.splitlines(): + content = line.split('#', 1)[0].rstrip() + if not content.strip(): + continue + indent = len(content) - len(content.lstrip()) + if indent == 0: + section, _, value = content.partition(':') + child_indent = None + if section == 'name': + pubspec['name'] = value.strip().strip('\'"') + continue + if child_indent is None: + child_indent = indent + if section == 'workspace' and content.lstrip().startswith('- '): + pubspec['workspace'].append(content.lstrip()[2:].strip().strip('\'"').rstrip('/')) + elif section in {'dependencies', 'dev_dependencies'} and indent == child_indent: + match = re.fullmatch(r'\s*([A-Za-z_][A-Za-z0-9_]*)\s*:.*', content) + if match: + pubspec['dependencies'].add(match.group(1)) + return pubspec + + +def load_workspace(root): + """Map each workspace member directory to its parsed pubspec.""" + members = read_pubspec((root / 'pubspec.yaml').read_text())['workspace'] + return {member: read_pubspec((root / member / 'pubspec.yaml').read_text()) for member in members} + + +def base_manifest_targets(root, base, head): + """Return the registered targets at the PR merge base, or none when the manifest did not exist.""" + merge_base = subprocess.check_output(['git', 'merge-base', base, head], cwd=root).decode().strip() + revision = f'{merge_base}:.github/integration-tests.json' + if subprocess.run(['git', 'cat-file', '-e', revision], cwd=root, capture_output=True).returncode: + return [] + return json.loads(subprocess.check_output(['git', 'show', revision], cwd=root))['targets'] + + +def select_targets(targets, paths, base_targets=(), workspace=None): if paths is None: return targets - shared = any( - path in {'pubspec.yaml', 'pubspec.lock', '.github/integration-tests.json'} - or path.startswith(('.github/workflows/', 'tool/ci/')) - or (path.startswith('packages/') and path.endswith('/pubspec.yaml')) + # Lockfile and CI changes can affect every job. + if any(path == 'pubspec.lock' or path.startswith(('.github/workflows/', 'tool/ci/')) for path in paths): + return targets + workspace = workspace or {} + packages = {path.split('/')[1] for path in paths if path.startswith('packages/') and path.count('/') > 1} + # A member pubspec change also affects workspace members that depend on it, directly or transitively. + names = { + workspace[path.removesuffix('/pubspec.yaml')]['name'] for path in paths - ) - return [target for target in targets if shared or any( - path.startswith(f'packages/{target["package"]}/') for path in paths + if path.endswith('/pubspec.yaml') and path.removesuffix('/pubspec.yaml') in workspace + } + while dependents := { + pubspec['name'] for pubspec in workspace.values() if pubspec['dependencies'] & names + } - names: + names |= dependents + packages |= { + member.split('/')[1] + for member, pubspec in workspace.items() + if member.startswith('packages/') and pubspec['name'] in names + } + # A manifest change affects only added or edited targets; removed targets need no job. + manifest_changed = '.github/integration-tests.json' in paths + return [target for target in targets if target['package'] in packages or ( + manifest_changed and target not in base_targets )] def main(): parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument('--base', help='PR base SHA; omit to select all targets') - parser.add_argument('--head', default='HEAD', help='PR head SHA') + parser.add_argument('--base', help='PR base or pushed-before SHA; omit to select all targets') + parser.add_argument('--head', default='HEAD', help='PR head or pushed SHA') parser.add_argument('--output', type=Path, help='GitHub Actions output file') args = parser.parse_args() targets = load_targets(ROOT) - paths = changed_files(ROOT, args.base, args.head) if args.base else None - selected = select_targets(targets, paths) + base = args.base and diffable_base(ROOT, args.base) + if args.base and not base: + print(f'Cannot diff from {args.base}; selecting all targets.', file=sys.stderr) + paths = changed_files(ROOT, base, args.head) if base else None + base_targets = base_manifest_targets(ROOT, base, args.head) if base else [] + selected = select_targets(targets, paths, base_targets, load_workspace(ROOT)) matrix = json.dumps({'include': selected}, separators=(',', ':')) print(matrix) if args.output: diff --git a/tool/ci/test_select_integration_targets.py b/tool/ci/test_select_integration_targets.py index 9fe59e76..692bb0ec 100644 --- a/tool/ci/test_select_integration_targets.py +++ b/tool/ci/test_select_integration_targets.py @@ -8,12 +8,22 @@ import tempfile import unittest -from select_integration_targets import ROOT, changed_files, load_targets, select_targets +from select_integration_targets import ( + ROOT, + base_manifest_targets, + changed_files, + diffable_base, + load_targets, + load_workspace, + read_pubspec, + select_targets, +) class TargetSelectionTest(unittest.TestCase): def setUp(self): self.targets = load_targets(ROOT) + self.workspace = load_workspace(ROOT) def test_related_and_unrelated_paths(self): native = [target for target in self.targets if target['package'] == 'desktop_webview_window'] @@ -26,16 +36,103 @@ def test_related_and_unrelated_paths(self): 'packages/desktop_drop/test/browser/web_drop_test.dart': browser, 'packages/desktop_webview_window_other/lib/test.dart': [], 'README.md': [], - 'pubspec.yaml': self.targets, + 'pubspec.yaml': [], 'pubspec.lock': self.targets, - 'packages/desktop_drop/pubspec.yaml': self.targets, - '.github/integration-tests.json': self.targets, + 'packages/desktop_drop/pubspec.yaml': browser, + 'packages/desktop_drop/example/pubspec.yaml': browser, + '.github/integration-tests.json': [], '.github/workflows/integration-tests.yml': self.targets, + '.github/workflows/ci.yml': self.targets, 'tool/ci/select_integration_targets.py': self.targets, } for path, expected in cases.items(): with self.subTest(path=path): - self.assertEqual(select_targets(self.targets, [path]), expected) + self.assertEqual(select_targets(self.targets, [path], self.targets, self.workspace), expected) + + def test_read_pubspec(self): + pubspec = read_pubspec('''name: 'sample' # comment +environment: + sdk: ^3.12.0 +workspace: + - packages/first + - "packages/first/example/" # trailing slash +dependencies: + flutter: + sdk: flutter + + # commented_out: ^1.0.0 + local: + path: ../local + hosted: ^1.0.0 +dev_dependencies: + lints: ^6.0.0 + git_package: + git: + url: https://example.invalid/repo.git#main +dependency_overrides: + overridden: ^1.0.0 +flutter: + plugin: + platforms: + linux: + pluginClass: SamplePlugin +''') + self.assertEqual(pubspec, { + 'name': 'sample', + 'workspace': ['packages/first', 'packages/first/example'], + 'dependencies': {'flutter', 'local', 'hosted', 'lints', 'git_package'}, + }) + + def test_real_workspace_dependencies(self): + self.assertEqual(self.workspace['packages/desktop_drop']['name'], 'desktop_drop') + self.assertIn('desktop_drop', self.workspace['packages/desktop_drop/example']['dependencies']) + targets = [ + {'package': 'mixin_logger', 'platform': 'linux'}, + {'package': 'desktop_multi_window', 'platform': 'linux'}, + {'package': 'win_toast', 'platform': 'windows'}, + {'package': 'desktop_drop', 'platform': 'web'}, + ] + # Both examples depend on mixin_logger. + self.assertEqual( + select_targets(targets, ['packages/mixin_logger/pubspec.yaml'], workspace=self.workspace), + targets[:3], + ) + + def test_package_pubspec_selects_transitive_dependents(self): + workspace = { + 'packages/base': {'name': 'base', 'workspace': [], 'dependencies': {'flutter'}}, + 'packages/base/example': {'name': 'base_example', 'workspace': [], 'dependencies': {'base'}}, + 'packages/middle': {'name': 'middle', 'workspace': [], 'dependencies': {'base'}}, + 'packages/top': {'name': 'top', 'workspace': [], 'dependencies': {'middle'}}, + 'packages/other': {'name': 'other', 'workspace': [], 'dependencies': {'flutter'}}, + } + targets = [{'package': package, 'platform': 'linux'} for package in ['base', 'middle', 'top', 'other']] + cases = { + 'packages/base/pubspec.yaml': targets[:3], + 'packages/base/example/pubspec.yaml': targets[:1], + 'packages/middle/pubspec.yaml': targets[1:3], + 'packages/top/pubspec.yaml': targets[2:3], + 'packages/base/lib/base.dart': targets[:1], + 'packages/removed/pubspec.yaml': [], + 'pubspec.yaml': [], + } + for path, expected in cases.items(): + with self.subTest(path=path): + self.assertEqual(select_targets(targets, [path], workspace=workspace), expected) + + def test_manifest_change_selects_added_and_edited_targets(self): + unchanged = {'package': 'unchanged', 'platform': 'linux', 'runner': 'ubuntu-latest'} + edited = {'package': 'edited', 'platform': 'macos', 'runner': 'macos-latest'} + removed = {'package': 'removed', 'platform': 'linux', 'runner': 'ubuntu-latest'} + added = {'package': 'added', 'platform': 'windows', 'runner': 'windows-latest'} + base = [unchanged, edited, removed] + head = [unchanged, {**edited, 'runner': 'macos-15'}, added] + manifest = '.github/integration-tests.json' + self.assertEqual(select_targets(head, [manifest], base), head[1:]) + self.assertEqual(select_targets(head, [manifest], head), []) + self.assertEqual(select_targets(head, [manifest], []), head) + # Unrelated targets in the base manifest do not matter when it is unchanged. + self.assertEqual(select_targets(head, ['packages/unchanged/lib/a.dart'], []), head[:1]) def test_all_and_empty(self): self.assertEqual(select_targets(self.targets, None), self.targets) @@ -118,6 +215,92 @@ def commit(path): with self.assertRaises(subprocess.CalledProcessError): changed_files(root, 'missing-base', 'HEAD') + def test_base_manifest_uses_merge_base_and_handles_missing_manifest(self): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + + def git(*args): + return subprocess.check_output(['git', *args], cwd=root, stderr=subprocess.DEVNULL).decode().strip() + + def commit(path, content): + file = root / path + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text(content) + git('add', '.') + git('commit', '-m', 'test fixture') + return git('rev-parse', 'HEAD') + + def manifest(*packages): + return json.dumps({'targets': [{'package': package, 'platform': 'linux'} for package in packages]}) + + git('init', '-b', 'main') + git('config', 'user.name', 'CI Test') + git('config', 'user.email', 'ci@example.invalid') + path = '.github/integration-tests.json' + original = commit('README.md', 'fixture') + head = commit(path, manifest('first')) + self.assertEqual(base_manifest_targets(root, original, head), []) + fork = commit(path, manifest('first', 'second')) + git('checkout', '-b', 'feature') + head = commit(path, manifest('first', 'second', 'third')) + git('checkout', 'main') + # The base branch edits the manifest after the PR forks; compare against the fork point. + base = commit(path, manifest('first', 'second', 'base_only')) + base_targets = base_manifest_targets(root, base, head) + self.assertEqual(base_targets, json.loads(manifest('first', 'second'))['targets']) + head_targets = json.loads(manifest('first', 'second', 'third'))['targets'] + self.assertEqual(select_targets(head_targets, [path], base_targets), head_targets[2:]) + self.assertEqual(fork, git('merge-base', base, head)) + + def test_push_base_falls_back_when_it_cannot_be_diffed(self): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + + def git(*args): + return subprocess.check_output(['git', *args], cwd=root, stderr=subprocess.DEVNULL).decode().strip() + + git('init', '-b', 'main') + git('config', 'user.name', 'CI Test') + git('config', 'user.email', 'ci@example.invalid') + (root / 'README.md').write_text('fixture') + git('add', '.') + git('commit', '-m', 'test fixture') + before = git('rev-parse', 'HEAD') + self.assertEqual(diffable_base(root, before), before) + # New branch. + self.assertIsNone(diffable_base(root, '0' * 40)) + # Force push that dropped the previous tip from the history. + self.assertIsNone(diffable_base(root, 'f' * 40)) + self.assertIsNone(diffable_base(root, git('rev-parse', 'HEAD^{tree}'))) + + def test_workflow_selection_arguments(self): + workflow = (ROOT / '.github/workflows/integration-tests.yml').read_text() + script = workflow.split(' - name: Select targets\n', 1)[1].split(' run: |\n', 1)[1] + script = '\n'.join(line[10:] for line in script.split('\n\n', 1)[0].splitlines()) + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + selector = root / 'tool/ci/select_integration_targets.py' + selector.parent.mkdir(parents=True) + selector.write_text('import sys\nprint(" ".join(sys.argv[1:]))\n') + env = { + **os.environ, + 'PR_BASE': 'pr-base', + 'PR_HEAD': 'pr-head', + 'PUSH_BEFORE': 'push-before', + 'GITHUB_SHA': 'pushed', + 'GITHUB_OUTPUT': 'output', + } + cases = { + 'pull_request': '--base pr-base --head pr-head --output output', + 'push': '--base push-before --head pushed --output output', + 'schedule': '--output output', + 'workflow_dispatch': '--output output', + } + for event, expected in cases.items(): + with self.subTest(event=event): + output = subprocess.check_output(['bash', '-e', '-c', script], cwd=root, env={**env, 'EVENT': event}) + self.assertEqual(output.decode().strip(), expected) + def test_invalid_manifest_fails_instead_of_skipping(self): with tempfile.TemporaryDirectory() as directory: root = Path(directory)