Repository navigation
ADFA-4739: Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action #24
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| name: Python tests | ||
|
|
||
| # Runs the repo's pytest suites on every push and PR. | ||
| # | ||
| # Added because nothing here executed them: the Kotlin-docs pipeline ships | ||
| # ~1,300 lines of regression tests covering permanent, silent data-loss paths | ||
| # (a migration deleting an unrelated page, an image optimizer collapsing two | ||
| # sources onto one output, a chunk chain reassembling truncated), and until now | ||
| # they only ran when someone remembered to run them locally. Tests that never | ||
| # run in CI rot, and these are exactly the ones whose failure is invisible | ||
| # without them. | ||
| # | ||
| # The suites are separate because their dependencies are: docdb-studio and | ||
| # check-tools each own a pyproject.toml + uv.lock, while ProcessKotlinWebsiteJSON | ||
|
Collaborator
There was a problem hiding this comment. F14 · Medium — this header explains a three-way split, but only two suites actually run The comment reads "docdb-studio and check-tools each own a pyproject.toml + uv.lock, while ProcessKotlinWebsiteJSON runs against the root requirements.txt", and the workflow's stated purpose is "Runs the repo's pytest suites on every push and PR". There are three suites' worth of tests in the repo and only two are executed.
Collaborator
Author
There was a problem hiding this comment. Half fixed in 46235fb, and I want to be explicit about the half that isn't. The header is corrected — it no longer describes a three-way split while running two. I did not add the check-tools step, because it would make every PR red immediately: 6 of its 13 tests already fail on So your premise is right — that suite runs nowhere, and it covers the checker behind the daily cron — but wiring it up needs those fixed first, which is its own change rather than something to smuggle in here. The workflow header now records that, with the exact step to add once they pass. Happy to take it as a follow-up if you'd rather it land before this merges. |
||
| # runs against the root requirements.txt. | ||
| # | ||
| # check-tools is NOT run here, deliberately. Its suite covers the health checker | ||
| # behind the daily docdb-regression-test.yaml cron and genuinely ought to be | ||
| # wired up - but 6 of its 13 tests already fail on main | ||
| # (test_db_health_checker.py: the checker reports no issues where the tests | ||
| # expect one), in code this PR doesn't touch. Adding the step as-is would make | ||
| # every PR red for an unrelated reason. Fixing those belongs in its own change; | ||
| # adding the step is a one-liner once they pass: | ||
| # - name: check-tools | ||
| # working-directory: check-tools | ||
| # run: uv run --frozen -- python -m pytest -q | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: python-tests-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| pytest: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 20 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.11' | ||
|
|
||
| - name: Install system dependencies | ||
| run: | | ||
| sudo apt-get update -y | ||
| # pngquant: optimize_media.py shells out to it, and its tests exercise | ||
| # the real binary rather than mocking it. | ||
| # brotli: the CLI, not the Python package - DictionaryCompressor uses | ||
| # it because no Python binding exposes a custom dictionary. | ||
| # zstd: train_dictionary uses `zstd --train-fastcover` to build the | ||
| # shared dictionary the migration tests need. | ||
| sudo apt-get install -y pngquant brotli zstd | ||
|
|
||
| - name: Set up uv | ||
| uses: astral-sh/setup-uv@v5 | ||
|
|
||
| - name: 'ProcessKotlinWebsiteJSON + sync_kotlin_stdlib_docs' | ||
| run: | | ||
| uv run --with-requirements requirements.txt --with pytest -- \ | ||
| python -m pytest \ | ||
| ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON \ | ||
| scripts/sync_kotlin_stdlib_docs \ | ||
| -q | ||
|
|
||
| - name: docdb-studio | ||
| working-directory: docdb-studio | ||
| run: uv run --frozen -- python -m pytest -q | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F14 · medium
The lock signalling is asymmetric. This "Grabbing baton" notification is ungated and fires on every run, while
'Notify Slack: build complete'at L427 carriesif: ${{ !inputs.dry_run }}plus the implicitsuccess().Since
dry_rundefaults totrue(L119), an ordinary run grabs the baton and never drops it — and so does any run that fails somewhere in the 3-hour build. The channel ends up showing a baton held forever by a dead run, which is the exact failure the convention exists to prevent.Gate the two notifications identically, and consider
if: always()on the release so a failed build still drops the baton.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d9df0f0 in both workflows. You're right that the asymmetry was the bug and that
dry_rundefaulting to true made it the normal case.Went with
if: always()on the release rather than matching the grab's gate: a failed build has to drop the baton too, which a plain!dry_runwouldn't do. Since it now always fires, the message reports the actual outcome — failed / dry run / updated — instead of always claiming an update.