fix(e2e): stop the export security tests asserting nothing - #390
Conversation
`sharedBlockId` was created once in `beforeAll`, but `afterEach` runs `removeAllBlocks`, which keeps only a block titled `Example Block`. The shared block was therefore deleted after the first test, and the two later tests read an id whose post no longer existed. They passed anyway, because a missing block denies access exactly as a permission check would. Created per test instead. The admin test's export-link assertions were wrapped in `if ( exportLinkCount > 0 )`, so if the export row action ever stopped rendering the test would report success having checked nothing -- and that link is the whole subject of the file. Now `toHaveCount(1)`. Deleted 'Permission verification - edit_lazyblocks required' outright. Its `page.evaluate` built an object literal and the three assertions then checked the values the test had just written; the only real thing exercised was that `window.ajaxurl` exists, via a full Dashboard load. The admin test also re-ran the creation wizard that block-builder-create-block.spec.js exists to cover -- Continue, Title, Continue, Finish, double Publish, read post_ID -- for a block it only needed to exist. Built over REST now, keeping the export assertions that are unique to this file. Speed, no coverage change: - openBlockBuilder loaded the block list and clicked the row title to reach a URL blockID already determines. It now navigates straight to post.php, which also removes a race against the list-table render. Runs 8 times per suite. - The unauthenticated test waited for `networkidle` after `goto`, but its response listener fires during that navigation, so the outcome was already settled. - `switchUserToAdmin` in a `finally`: `page` is test-scoped and Playwright closes it at teardown, and the next test opens a fresh context already authenticated from storageState. - editor-block-component-inner-blocks created a byte-identical block in all three tests and mutated none of them. Also drops the build step from the lint job -- `lint:js` is `biome check`, whose shared config excludes `**/build`, and `lint:css` globs the SCSS sources, so neither reads build output -- and picks up @nk-crew/plugin-toolkit 0.6.0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 756f45af16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(exportHref).toContain('lazyblocks_export_nonce='); | ||
| } | ||
| }); | ||
| await expect(exportLink).toHaveCount(1); |
There was a problem hiding this comment.
Restore the export action visibility assertion
When a CSS or row-action-state regression leaves this anchor rendered but inaccessible, the count and href assertions still pass, so the test named “Admin can create and export blocks” no longer verifies that an administrator can reach the export action. The previous conditional branch called toBeVisible() whenever the link existed; retain that assertion after making the count unconditional.
Useful? React with 👍 / 👎.
Toolkit 0.6.0 plus fixes from profiling this suite. The correctness half matters more than the seconds.
sharedBlockIdpointed at a deleted post for two of three testsbeforeAllcreated the block once.afterEachrunsremoveAllBlocks, which keeps only a block titledExample Block(utils/remove-all-blocks.js:17) — and this one isShared Security Test Block. So it was deleted after the first test, and the later tests read an id whose post no longer existed.They passed anyway: a missing block denies access exactly as a permission check would. Moved to
beforeEach.Assertions that could not fail
if (exportLinkCount > 0) { … }— if the export row action ever stopped rendering, the block was skipped and the test reported success having verified nothing. That link is the entire subject of the file. Nowawait expect(exportLink).toHaveCount(1).Permission verification - edit_lazyblocks requiredis deleted. Itspage.evaluatebuilt an object literal and the three assertions then checked values the test itself had just written. The only real thing exercised was thatwindow.ajaxurlexists on an admin page — via a full Dashboard load.Duplicated wizard
The admin test re-ran the creation wizard that
block-builder-create-block.spec.jsexists to cover — Continue → Title → Continue → Finish → double Publish → readpost_ID— for a block it only needed to exist. Built over REST now; the export-link assertions unique to this file stay.Speed, no coverage change
openBlockBuilderwent via the block list, loadingedit.php?post_type=lazyblocksand clicking the row title to reach a URLblockIDalready determines — a full list-table render plus thepage.content()scanvisitAdminPageruns, 8 times per suite. It also raced the list render, so the click could fire before the row existed. Straight topost.phpnow. That the block is listed and links there is still asserted directly in three other specs.networkidleaftergotoin the unauthenticated test: the response listener registered above fires during that navigation, redirect chain included, so the outcome is already settled. The sibling contributor test does the samegotowithout one.switchUserToAdminin afinally:pageis a test-scoped fixture Playwright closes at teardown, and the next test opens a fresh context already authenticated fromstorageState. The login round trip bought nothing.editor-block-component-inner-blockscreated a byte-identical block in all three tests and mutated none of them. OnebeforeAll; the teardown stays, since a leftoverlazyblock/testregistration would follow the worker into the next file.Also
lintjob.lint:jsisbiome check, and the shared Biome config excludes**/build;lint:cssglobsassets/**/*.scss. Neither reads build output. 8–15 s per PR.@nk-crew/plugin-toolkit→0.6.0(toolkit#2). Lockfile touched surgically — four fields for that one package;npm install --package-lock-onlyre-resolves the whole tree and on a sibling repo dropped two optional peers.Flagged, not fixed — the two "cannot export" tests never reach the capability check
maybe_export_jsonis hooked toadmin_init(classes/class-tools.php:33) and starts with:The capability gate these tests exist to protect —
current_user_can( 'edit_lazyblocks' )at:527— sits after thatwp_die. Both tests deliberately send a fake nonce (invalid_nonce_12345,invalid_nonce_contributor), so they short-circuit at the nonce check and never execute it. Revertedit_lazyblockstoread_lazyblock— the exact regression ofbd70501— and both still pass. The contributor test's content assertion even matches'Export permission denied', which is the nonce message.I did not attempt a fix, and I'd flag that the obvious one does not work either: WordPress nonces are bound to the user, so scraping a valid nonce as admin and replaying it as a contributor fails the same check. For the contributor to hold a valid nonce they would have to reach a screen that mints one — which may mean the nonce gate is the real boundary here and the capability check is unreachable for that role. That is a design question for whoever wrote the fix, not something to guess at in a CI pass.