-
-
Notifications
You must be signed in to change notification settings - Fork 1
ADFA-4739: Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
alexmmiller
wants to merge
44
commits into
main
Choose a base branch
from
fix/ADFA-4739
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
a4795f1
Convert Kotlin website Writerside content to JSON, add it to database…
alexmmiller 3b128c0
Kotlin docs sync and website processing end-to-end pipeline test
alexmmiller e5ff684
Config/assets for pipeline test
alexmmiller cd59353
Address PR review feedback and add blacklist-pruning verification
alexmmiller ae44822
Merge branch 'fix/ADFA-4514' into fix/ADFA-4737
alexmmiller 5984357
Automate stdlib JSON doc generation in the e2e pipeline script
alexmmiller 1f7edd4
Add repository-root CLAUDE.md
alexmmiller 9b4ba07
Add Build Kotlin Docs workflow
alexmmiller 421e529
Add Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action (ADFA-4…
alexmmiller 66da59d
Address Hal's PR #24 review feedback
1cf41d2
Merge remote-tracking branch 'origin/fix/ADFA-5039' into fix/ADFA-4739
26cb831
Fix find_include_warnings crashing on a non-UTF-8 file
26c6250
Compress Kotlin-website Content rows against a shared Brotli dictionary
davidschachterADFA 09ca170
Add whole-database migration to shared-dictionary Brotli
davidschachterADFA 97755b1
Make docdb-studio's Content reads/writes dictionary-aware
davidschachterADFA 2827bfb
Parallelize the whole-database migration's read+compress phase
davidschachterADFA b203500
ADFA-5141: Pin page_size in populate_db.py's own VACUUM
davidschachterADFA b09331f
ADFA-5141: Fix the same WAL deadlock in populate_db.py's own VACUUM
davidschachterADFA b5084b5
ADFA-5141: Restore file permissions after the VACUUM INTO swap
davidschachterADFA 7970cdd
ADFA-5141: Use a bound parameter for VACUUM INTO's target, close jour…
davidschachterADFA dda6410
ADFA-5141: Fix chmod ordering and unclosed connections, matching PR #25
davidschachterADFA 801f5eb
Revert ADFA-5141 page_size pinning: declined, keeping this PR scoped …
davidschachterADFA 358276d
ADFA-5171: Add a repair script for chunked rows misnumbered from -2
davidschachterADFA 0599a37
Merge pull request #27 from appdevforall/fix/ADFA-5171-fragment-renum…
davidschachterADFA 838ac44
ADFA-5153: Address review findings on the dictionary migration
davidschachterADFA 4d4f37d
ADFA-5153: Route the last LIKE delete through fragment_chain, declare…
davidschachterADFA 4b19f14
ADFA-5153: Add the dictionary re-mint tooling used on the 21-Aug data…
davidschachterADFA 25d284f
ADFA-5153: Tell Windows users how to install the brotli CLI, and mean it
davidschachterADFA 5a00e22
Merge pull request #26 from appdevforall/ADFA-5153-content-brotli-dic…
davidschachterADFA 7499935
Fix 9 issues from the PR #24 database-insertion review
8ace4f4
Merge fix/ADFA-4737 into fix/ADFA-4739 (shared-Brotli-dictionary pipe…
80e234a
Merge fix/ADFA-5039 into fix/ADFA-4739 (md_to_json.py review fixes)
06a43f5
Make the Build Kotlin Docs pipeline actually runnable end-to-end
alexmmiller d9df0f0
Fix the 15 findings from Hal's PR #24 review, with regression tests
alexmmiller e6786a8
Merge origin/main into fix/ADFA-4739
alexmmiller 46235fb
Fix the 14 findings from Hal's second PR #24 review
alexmmiller a59d777
Fix the five findings from the /code-review pass
alexmmiller 0204dbc
Fix the findings from Hal's third PR #24 review
alexmmiller fa1848f
Fix the eight findings from the /code-review xhigh pass
alexmmiller 63a2dea
Fix the eight findings from the second /code-review xhigh pass
alexmmiller 966fb90
Fix the seven findings from the third /code-review xhigh pass
alexmmiller 6b5345c
Fix the seven findings from the fourth /code-review xhigh pass
alexmmiller 38caac4
Fix the seven findings from the fifth /code-review xhigh pass
alexmmiller 99c9b2f
Fix the seven findings from the sixth /code-review xhigh pass
alexmmiller File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
| # 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 | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 — 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.
check-tools/tests/test_db_health_checker.py— covering the health checker wired into the dailydocdb-regression-test.yamlcron — continues to run nowhere. Add auv run --frozen -- python -m pytest -qstep withworking-directory: check-tools, matching the docdb-studio step.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.
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
main, in code this PR doesn't touch.test_db_health_checker.pyexpects one issue from each of the empty-content, orphaned-tooltip and broken-URI checks and gets zero: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.