From ae114e9636a2c50e071747ca56f078760ec6bcdd Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Fri, 25 Sep 2026 16:44:05 +1200 Subject: [PATCH] ci: run the Tools suites that read product source on every change, and filter only the leg that does not `tools-python.yml` carried a `Tools/**` path filter, so a change touching no Python never ran it. Its own header already argued against that, in detail: the core Tools/ suites assert on the CONTENTS of app and package files, `test_home_i18n.py` reads TodayScreen.kt, AppModel.swift and the xcstrings catalogues, and a Swift-only commit had already broken it once while the filter kept the job from running, leaving main red until an unrelated contributor touching Tools/ tripped over it in their own PR (#1691). The filter was reinstated underneath that paragraph anyway, for a reason that was sound on its own terms: a Windows runner billed at twice a Linux minute should not run on the many pull requests that touch no Python. It then cost the same thing a second time. On 2026-09-24 an Android-only commit changed the hero label padding that `test_home_i18n.py` asserts on, this job did not run, and main carried a red suite through nine merges until a commit that happened to touch Tools/ finally tripped it. One of those nine was an outside contributor's. So the two legs are split rather than the argument had a third time: tools-python.yml UNFILTERED. Runs the Tools/linux-capture decoders AND the core Tools/ suites, and the core suites read product source, so no path list can describe its inputs. Ubuntu, about 25 seconds, every PR and every push. tools-python-windows.yml FILTERED on Tools/linux-capture/**. Runs ONLY the linux-capture tests, a second time under legacy Windows console encodings, and those read nothing outside their own package. The cost argument is true here and is answered here. Job names are unchanged, `linux-capture` and `windows-capture`, so the check names are too. `test_core_tools_filter_covers_every_governance_tool_path` pinned the old filter exactly, so it is repinned to the new shape rather than dropped: the core workflow must carry NO path filter and must still run the core Tools/ step, and the Windows workflow must carry exactly the linux-capture filter and must NOT run the core step. Reinstating `Tools/**` on the core leg now reds it, which is what the last two rounds of this lacked. Parity governance is left alone. Its filter is deliberate, there is a test asserting it excludes product globs, and it already carries a daily schedule on main for exactly the staleness a filter cannot see. Three documents described this and two of them were already describing the state this restores, not the state that shipped. CONTRIBUTING.md's check table said `linux-capture` runs on "every PR", and docs/CONTRIBUTING.md said `source-hygiene`, `tools-python` and `i18n-coverage` "have no path filter, so all three run on everything". Both were false under the filter: contributors were told a gate covered them while it was silent. They are true again now. AGENTS.md was the one that was accurate, "active, path-filtered", and it is updated, along with a row for the new Windows workflow that its table had never carried. CONTRIBUTING.md's table gains the `windows-capture` check it had never listed either. Core Tools suite 130 tests, no failures. Governance acceptance 49 tests, no failures. --- .github/workflows/tools-python-windows.yml | 67 +++++++++++++++++++ .github/workflows/tools-python.yml | 60 +++++------------ AGENTS.md | 3 +- CONTRIBUTING.md | 1 + .../test_parity_governance_acceptance.py | 34 ++++++++-- docs/CONTRIBUTING.md | 4 +- 6 files changed, 120 insertions(+), 49 deletions(-) create mode 100644 .github/workflows/tools-python-windows.yml diff --git a/.github/workflows/tools-python-windows.yml b/.github/workflows/tools-python-windows.yml new file mode 100644 index 0000000000..a9a2581960 --- /dev/null +++ b/.github/workflows/tools-python-windows.yml @@ -0,0 +1,67 @@ +# The Windows leg of the Tools/ Python suites, split out of tools-python.yml so each trigger states +# something true. +# +# `Tools/linux-capture` is run a second time under legacy Windows console encodings, which is what +# catches an implicit file encoding or a console write that only fails off UTF-8. It is the same 200+ +# tests, on a runner billed at twice a Linux minute. +# +# Path-filtered, and here that is correct: this job runs ONLY the tests under Tools/linux-capture, and +# those read nothing outside their own package. That is what separates it from the Linux leg, which also +# runs the core Tools/ suites, and those DO assert on app and package source (test_home_i18n.py reads +# TodayScreen.kt, AppModel.swift, the xcstrings catalogues). A filter on the shared workflow silenced +# both; this one is scoped to the job it was ever meant for. +# +# If a test that reads product source is ever added under Tools/linux-capture, this filter becomes wrong +# in the same way. Put such a test in Tools/ instead, where the unfiltered leg runs it. +name: Tools Python CI (Windows) + +on: + pull_request: + branches: [main] + paths: + - 'Tools/linux-capture/**' + - '.github/workflows/tools-python-windows.yml' + push: + branches: [main] + paths: + - 'Tools/linux-capture/**' + - '.github/workflows/tools-python-windows.yml' + workflow_dispatch: + +concurrency: + group: tools-python-windows-${{ github.ref }} + cancel-in-progress: true + +jobs: + windows-capture: + runs-on: windows-latest + steps: + - name: Checkout + uses: actions/checkout@v5 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + + # Keep Windows locale defaults and exercise a strict legacy console, including subprocesses. + # UTF-8 mode would hide implicit file encodings; replacement output would hide console bugs. + - name: Run Tools/linux-capture tests with legacy Windows encodings + working-directory: Tools/linux-capture + env: + PYTHONUTF8: '0' + PYTHONIOENCODING: 'cp1252:strict' + PYTHONLEGACYWINDOWSSTDIO: '1' + shell: python + run: | + import sys + import unittest + + suite = unittest.defaultTestLoader.discover('.', pattern='test_*.py') + count = suite.countTestCases() + print(f'collected {count} tests', flush=True) + result = unittest.TextTestRunner(verbosity=2).run(suite) + if count < 200 or result.testsRun < 200: + print('::error::expected at least 200 tests; discovery or execution is incomplete') + sys.exit(1) + sys.exit(0 if result.wasSuccessful() else 1) diff --git a/.github/workflows/tools-python.yml b/.github/workflows/tools-python.yml index 1479de962d..2b4868379f 100644 --- a/.github/workflows/tools-python.yml +++ b/.github/workflows/tools-python.yml @@ -18,23 +18,31 @@ # Widening the filter to name those paths would just re-create the trap one edit later: the list is # whatever the assertions happen to read today. Twenty-five seconds on every PR is cheaper than a gate # that is green because it did not run. +# +# That paragraph was already here, and a `Tools/**` filter was reinstated underneath it anyway, on the +# Windows runner's cost. It cost the same thing a second time on 2026-09-24: an Android-only commit +# changed a padding `test_home_i18n.py` asserts on, this job did not run, and main carried a red suite +# through nine merges until a commit that happened to touch Tools/ finally tripped it. One of those +# merges was an outside contributor's. +# +# So the split below, rather than a third round of the same argument: the Linux leg runs on everything +# because it reads product source, and the Windows leg keeps its filter because it does not. name: Tools Python CI on: - # Path-filtered like android.yml / swift-packages.yml / app-build.yml. Without this the workflow ran on - # EVERY pull request and every push to main, including the many that touch no Python at all - and with a - # Windows runner now in it, billed at twice a Linux minute, that is a real cost for a job whose inputs - # cannot have changed. The workflow file itself is included so a change to the job still tests itself. + # UNFILTERED, which is what the note above argues for and what the filter that used to sit here undid. + # + # The filter was added for a real reason: a Windows runner billed at twice a Linux minute should not run + # on the many pull requests that touch no Python. That reason was sound and the remedy was not, because + # it silenced the Linux leg too, and the Linux leg is the one that reads product source. The Windows leg + # now lives in tools-python-windows.yml and keeps a filter of its own, so the cost argument is answered + # where it applies instead of across a job it was never about. + # + # This leg is ubuntu and about 25 seconds. It stays on every pull request and every push to main. pull_request: branches: [main] - paths: - - 'Tools/**' - - '.github/workflows/tools-python.yml' push: branches: [main] - paths: - - 'Tools/**' - - '.github/workflows/tools-python.yml' workflow_dispatch: concurrency: @@ -89,35 +97,3 @@ jobs: exit 1 fi working-directory: Tools - windows-capture: - runs-on: windows-latest - steps: - - name: Checkout - uses: actions/checkout@v5 - - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: '3.12' - - # Keep Windows locale defaults and exercise a strict legacy console, including subprocesses. - # UTF-8 mode would hide implicit file encodings; replacement output would hide console bugs. - - name: Run Tools/linux-capture tests with legacy Windows encodings - working-directory: Tools/linux-capture - env: - PYTHONUTF8: '0' - PYTHONIOENCODING: 'cp1252:strict' - PYTHONLEGACYWINDOWSSTDIO: '1' - shell: python - run: | - import sys - import unittest - - suite = unittest.defaultTestLoader.discover('.', pattern='test_*.py') - count = suite.countTestCases() - print(f'collected {count} tests', flush=True) - result = unittest.TextTestRunner(verbosity=2).run(suite) - if count < 200 or result.testsRun < 200: - print('::error::expected at least 200 tests; discovery or execution is incomplete') - sys.exit(1) - sys.exit(0 if result.wasSuccessful() else 1) diff --git a/AGENTS.md b/AGENTS.md index e7756815e3..42d4313f33 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -124,7 +124,8 @@ xcodegen generate && xcodebuild -project Strand.xcodeproj -scheme Strand \ | `android.yml` | `assembleFullDebug` + `testFullDebugUnitTest` | ubuntu | **active**, path-filtered to `android/**` | | `source-hygiene.yml` | Doc comments that bind to nothing (`Tools/doc_comment_lint.py`) | ubuntu | **active** | | `i18n-coverage.yml` | Diff-scoped translation gate (`Tools/i18n_audit.py --ci`) | ubuntu | **active** | -| `tools-python.yml` | `unittest discover` over `Tools/` and `Tools/linux-capture` | ubuntu | **active**, path-filtered | +| `tools-python.yml` | `unittest discover` over `Tools/` and `Tools/linux-capture` | ubuntu | **active**, NO path filter. The core `Tools/` suites assert on product source (`test_home_i18n.py` reads `TodayScreen.kt`, `AppModel.swift`, the xcstrings catalogues), so no path list describes its inputs. Runs on every PR and every push to `main`. | +| `tools-python-windows.yml` | The `Tools/linux-capture` tests a second time, under legacy Windows console encodings | windows | **active**, path-filtered to `Tools/linux-capture/**`. Those tests read nothing outside their own package, and the runner bills at twice a Linux minute. | | `prune-stale-branches.yml` | Deletes branches whose PR merged or closed unmerged | ubuntu | **active**, weekly + dispatch | | `fork-testing-build.yml` / `fork-release.yml` | Staging / release builds (apk + mac + ios) | — | on dispatch | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 704bc97ba0..527241e385 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,6 +74,7 @@ names. That column is why this table exists: | `check` | **i18n Coverage** (`i18n-coverage.yml`) | every PR | | `doc-comments` | **Source Hygiene** (`source-hygiene.yml`) | every PR | | `linux-capture` | **Tools Python CI** (`tools-python.yml`) | every PR | +| `windows-capture` | **Tools Python CI (Windows)** (`tools-python-windows.yml`) | `Tools/linux-capture/**` | | `build-and-test` | **Android CI** (`android.yml`) | `android/**`, the protocol/store test resources, `Strand/Resources/Localizable.xcstrings` | | `test (…)`, `tools (…)` | **Swift Packages CI** (`swift-packages.yml`) | `Packages/**`, the `Tools/SleepBench`, `Tools/SleepPSG` and `Tools/Backfill` packages, `android/app/src/test/resources/**`, `Strand/Liquid/LiquidCore.swift` | diff --git a/Tools/tests/test_parity_governance_acceptance.py b/Tools/tests/test_parity_governance_acceptance.py index b449d45793..cc4d6a4abd 100644 --- a/Tools/tests/test_parity_governance_acceptance.py +++ b/Tools/tests/test_parity_governance_acceptance.py @@ -105,20 +105,45 @@ def test_core_tools_filter_covers_every_governance_tool_path(self) -> None: core = (REPOSITORY / ".github/workflows/tools-python.yml").read_text( encoding="utf-8" ) + windows = (REPOSITORY / ".github/workflows/tools-python-windows.yml").read_text( + encoding="utf-8" + ) governance = (REPOSITORY / ".github/workflows/parity-governance.yml").read_text( encoding="utf-8" ) - self.assertIn("pull_request:\n branches: [main]\n paths:", core) + # The core leg carries NO path filter, and that is the contract. + # + # It runs the Tools/ suites, and some of those assert on product source: test_home_i18n.py reads + # TodayScreen.kt, AppModel.swift and the xcstrings catalogues. A filter naming Tools/ therefore + # silences a suite whose inputs it does not describe. That was diagnosed once (#1691), written + # into the workflow header, and a Tools/** filter was reinstated under it anyway; on 2026-09-24 + # an Android-only commit broke test_home_i18n.py and main carried it red through nine merges. + # Naming the product paths instead would only move the trap, since the list is whatever the + # assertions happen to read today. + self.assertNotIn("pull_request:\n branches: [main]\n paths:", core) + self.assertNotIn("push:\n branches: [main]\n paths:", core) core_paths = [ line.strip()[3:-1] for line in core.splitlines() if line.startswith(" - '") ] + self.assertEqual([], core_paths) + self.assertIn("working-directory: Tools\n", core) + self.assertNotIn("unittest discover -s tests", core) + # The Windows leg keeps a filter, because it runs ONLY the Tools/linux-capture tests and those + # read nothing outside their own package. That is the whole reason it could be split off: the + # runner costs twice a Linux minute, and the cost argument is true for this job alone. + windows_paths = [ + line.strip()[3:-1] + for line in windows.splitlines() + if line.startswith(" - '") + ] self.assertEqual( - ["Tools/**", ".github/workflows/tools-python.yml"] * 2, - core_paths, + ["Tools/linux-capture/**", ".github/workflows/tools-python-windows.yml"] * 2, + windows_paths, ) - self.assertNotIn("unittest discover -s tests", core) + self.assertIn("working-directory: Tools/linux-capture", windows) + self.assertNotIn("working-directory: Tools\n", windows) self.assertIn("pull_request:\n branches: [main]\n paths:", governance) governance_paths = [ line.strip()[3:-1] @@ -137,7 +162,6 @@ def test_core_tools_filter_covers_every_governance_tool_path(self) -> None: ] * 2, governance_paths, ) - self.assertIn("Tools/**", core_paths) self.assertTrue( all( path.startswith("Tools/") diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 544e14311f..4accfe626c 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -255,7 +255,9 @@ and hardware-dependent verification runs at release time or on demand. This is a anonymous, offline, sideloaded project — not a gap to fill with more gates. - **On every PR (required):** `source-hygiene`, `tools-python` and `i18n-coverage` have no path - filter, so all three run on everything. `swift-packages` (`swift test` for `Packages/**`) and + filter, so all three run on everything. (`tools-python`'s Windows leg is a separate workflow and IS + filtered, to `Tools/linux-capture/**`: it re-runs only those tests under legacy console encodings, + and they read nothing outside their own package.) `swift-packages` (`swift test` for `Packages/**`) and `android` (`assembleFullDebug` + `testFullDebugUnitTest`) are **path-filtered** — they run when you touch what they cover, which is most substantive PRs. Between them these catch the regressions that matter most (protocol/analytics math, storage, i18n) without a device or an app build. The check