Repository navigation
Conversation
`playwright/` held four specs that nothing ran: `make test` called an npm script that does not exist, and two of the four shelled out to `shinylive export` from the Python shinylive package -- which depends on this repository, the circular dependency that kept the playwright job in build.yml commented out for years. All four are now pytest tests in `tests/`, alongside the example suite, and they run in CI. Groundwork, which posit-dev#240 also needs: - `fail_on_example_errors` becomes `fail_on_page_errors`, and takes an opt-out: a test that provokes an error on purpose marks itself `allow_page_errors` and asserts on the failure instead. - Every test now carries an `examples` or a `site` marker. The example targets select on `examples`, so the site tests are not swept into their engine x shard matrix, and `make site-test` runs them in one job. The export fixture, `tests/export_app.py`: an export is `app.json`, `export_template/index.html` with its variables filled in, `edit/index.html` as-is, and the shinylive bundle -- all of which `make all` already produces under `build/`. Nothing here needs `pip install shinylive` any more. The migrated tests: - `test_site_editor.py` and `test_site_url_loading.py` from examples-viewer.spec.ts and load-from-url.spec.ts, against the `_shinylive/` build the session fixture already serves. - `test_site_export.py` from shiny-static.spec.ts and editor-cell.spec.ts, against the export fixture. The app mode comes from the `?_shinylive-mode=` query param, replacing a `sed` in playwright.config.ts that had not matched anything in `export_template/index.html` in a long time. Two fixes the migration turned up: load-from-url's "app view can show header bar" test navigated to `/editor/`, so it never tested the app view; and Mod-Enter in the editor is Cmd on a Mac, which the old spec worked around by pressing both modifiers. Removals: `playwright.config.ts`, the four specs and their helpers, the `cypress:open` script (cypress is not a dependency), the `@playwright/test` devDependency, the broken `test` target, and the commented-out playwright job in build.yml with its now-obsolete rationale.
Four follow-ups to the playwright migration, none of them changing what the migrated tests assert. The console-error gate would have failed on CI. build.yml is the only workflow that sets GOOGLE_TAG_MANAGER_ID, so `make all` there templates a Google Tag Manager loader into the site's pages (scripts/build.ts) -- and the site tests are the only suite that runs against that build. A tag manager that fails to load reaches the console as an error, exactly the way the missing favicon the gate already forgives does, so the suite that now gates the deploy would have depended on an outside host being reachable. `block_analytics` aborts those requests, which also keeps the async loader from holding up the page's load event, and the gate forgives the abort. A test with neither the `examples` nor the `site` marker runs in no CI job. `pytest.ini` says every test carries one, but nothing made that true; collection now refuses a test that has neither rather than let it silently never run. `--shard` was dealing out the whole of `tests/`, not the selected engine. A conftest's `pytest_collection_modifyitems` runs before the one in `_pytest.mark` -- which is what lets `-m py` see the engine markers added there -- so the sharding at the end of it was splitting all 91 tests and only then dropping the other engine's. The shards come out the same size today by coincidence; moving the sharding into a `trylast` plugin makes that hold whatever else lands in tests/. Also: `make help` never listed `test-deps`, because the `##` line has to sit directly above the target, and README now tells people to run it.
Contributor
Author
|
Superseded by #247 — same commits, from a branch on posit-dev rather than a fork. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #243.
playwright/held four specs that nothing ran.make testcallednpm run playwright test, a script that does not exist, and two of the four shelled out toshinylive exportfrom the Python shinylive package — which depends on this repository, the circular dependency that kept the playwright job inbuild.ymlcommented out for years. All four are now pytest tests intests/, next to the example suite #238 established, and they run in CI where a failure blocks the merge.Stacked conceptually on #244 (the jest harness, #242). The two overlap only in
package.jsonand the README's suite listing, so expect a small conflict in whichever lands second.Groundwork (#240 needs this too)
fail_on_example_errorsis nowfail_on_page_errorsand takes an opt-out: a test that provokes an error on purpose marks itself@pytest.mark.allow_page_errorsand asserts on the failure instead of being failed by the gate.Every test also carries an
examplesor asitemarker, and the example Make targets select onexamples, so a non-example test is no longer swept into the engine × shard matrix intest-apps.yml.make site-testruns the other side in one job. The marker is enforced at collection rather than documented — sincemake examples-smoke-testselects-m "examples and py"andmake site-testselects-m site, a file with neither marker is deselected by both and would silently run in no CI job at all. Collection now errors on one.Two things the review turned up in this area, both fixed here:
GOOGLE_TAG_MANAGER_IDset.build.ymlis the only workflow that sets it, somake allthere templates a Google Tag Manager loader into the site pages;test-apps.ymlnever has, which is why the examples suite has never met it. A tag manager that fails to load is a console error, and the gate would have failed on it — a hard dependency on a third-party host inside the workflow that gates the deploy. An autouse fixture now aborts analytics requests and the gate forgives the abort. Measured overhead on a Pyodide-booting test: none.--shardwas splitting the whole oftests/, not the selected engine. A conftest'spytest_collection_modifyitemsruns before_pytest.mark's, so the sharding was dealing out all 91 tests and only then dropping the other engine's. Today's shard sizes come out identical either way, but this PR is what put a second suite into that collection, so the balance was luck. Sharding moved into atrylastplugin. Engine selection and shard sizes are byte-identical tofe8c0b6: 54py, 22r, shards 18/18/18 and 8/7/7, and the shard union equals the full selection.The export fixture
tests/export_app.pyassembles a static export out of whatmake allalready produces underbuild/:app.json(the files, verbatim),export_template/index.htmlwith its variables filled in,edit/index.htmlas-is, and the shinylive bundle, symlinked rather than copied. That is everythingrunExportedApp()reads, so nothing in this repository needspip install shinyliveany more, and the stale rationale comment inbuild.ymlgoes away with it. Theexported_appfixture writes one and hands back the URL it is served at.The migrated tests
test_site_editor.pyandtest_site_url_loading.py, fromexamples-viewer.spec.tsandload-from-url.spec.ts, against the_shinylive/build the sessionstatic_serverfixture already serves.test_site_export.py, fromshiny-static.spec.tsandeditor-cell.spec.ts, against the export fixture. The app mode comes from?_shinylive-mode=, which replaces thesed -e 's/viewer/editor-cell/'inplaywright.config.tsthat had not matched anything inexport_template/index.htmlin a long time — the drift the issue describes.Three things the migration turned up in the originals:
expect_terminal_has_text()(playwright/helpers.ts:73) returned a boolean that every call site discarded, so both terminal assertions inexamples-viewer.spec.tspassed vacuously. The migrated helper actually asserts./editor/, so it had never tested the app view. It now goes to/app/.Mod-Enterin the editor is Cmd on a Mac, which the old spec papered over by pressing both modifiers.MOD_KEYpicks by platform; both branches were checked against the real editor, including theControlone CI takes.Removals
playwright.config.ts, the four specs and their helpers and fixture apps, thecypress:openscript (cypress is not a dependency), the@playwright/testdevDependency, the brokentesttarget, and the commented-out playwright job inbuild.yml.examples-test-depsbecomestest-deps, since both suites use it.CI
The site tests run in
build.yml, immediately aftermake alland before the deploy — that is what they test, it reuses a build that already takes most of the job's time, and it is where the disabled playwright job used to sit.Worth stating plainly: this means a flaky site test blocks the S3 deploy and the release-asset upload, not just the merge. That is what #243 asks for, and it is the same failure shape that got the old job commented out, minus the circular dependency that made it unfixable. It also adds several minutes to a job that currently runs in about 2–4 minutes:
test-depsbuilds Shiny from the submodule and downloads Chromium, and the tests themselves take ~55s. If that trade is wrong, the alternative is a seventh leg ontest-apps.yml, which costs a second fullmake all.The examples matrix in
test-apps.ymlis unchanged apart from the marker selection.Verification
All 15 site tests pass against a local build, over three runs (~55s each). The error gate was checked in all four directions: it catches console errors, catches page errors,
allow_page_errorsgenuinely skips both, and a clean page passes. The GTM failure was reproduced against a real GTM-templated page and confirmed fixed.make -n site-testandmake -n examples-smoke-testboth expand correctly and thetest-depsrename doesn't breaktest-apps.yml.npm ls --package-lock-only --allexits 0 and a throwawaynpm ciinstalls with zero playwright entries.The example suite still collects 91 and passes on a sample of both engines. One example test,
test_app_with_plot, fails against a local_shinylive/that predates theinput_slidermin change made infe8c0b6; it fails identically onmain, and CI builds fresh.Known gaps
Gate violations surface as pytest teardown ERRORs rather than FAILs — pre-existing from #238; the exit code is still non-zero, so CI blocks. The site suite covers only the Python site, same as the originals it replaces.