Raise coverage to ~96.7% and fix three defects found on the way - #88
Merged
Conversation
assets/js/exelearning.js and assets/js/exelearning-admin.js each held a single console.log inside a jQuery IIFE. No wp_enqueue_script or wp_register_script call references either path -- every enqueue in the plugin names its asset literally -- so neither file has ever reached a browser. They were still measured, and reported as two uncovered files.
The bridge is the editor's half of the postMessage protocol: it answers the parent modal's WP_REQUEST_SAVE / WP_REQUEST_EXPORT / GET_PROJECT_INFO / CONFIGURE calls from inside the embedded eXeLearning page. It was the largest untested file in the plugin at 163 lines and 0% coverage. The editor itself cannot exist under Vitest, so it is stubbed, but the bridge's own logic is real and it is the part that breaks: a requestId that is not echoed back, an error swallowed instead of reported, or a request that never gets an answer all leave the modal spinning with no way out. Every failure path now ends in a message, including the three polling timeouts, which are driven with fake timers rather than waited on. Also covered: the guard that keeps the bridge from answering its own messages, the parentOrigin handling, and DOCUMENT_CHANGED firing once per document rather than once per keystroke. The script is an IIFE that runs init() on import, so each test imports a fresh copy and its listeners are recorded and unbound afterwards -- otherwise an instance that binds late (one waiting on eXeLearning.ready) keeps answering messages in every later test. 163 lines: 0% -> 98.77%. What is left is one `if (!document.body)` guard and a catch that cannot be reached without breaking init first.
Half of this file was untested: the format menu and the .elpx download were covered, the export-through-a-hidden-iframe path was not. The file's own header explained why -- the handshake needs an iframe that really loads the editor, and faking that would test the fake. That reasoning holds for the editor's half and it stays in the Playwright suite. It does not hold for this half. The bootstrap URL, the readiness handshake, matching an answer to the request that asked for it and the two deadlines are ordinary bookkeeping with ordinary bugs, and none of them need a real editor to go wrong. Three of the new tests are about answers the page must refuse: one from another window, one for a request nobody made, and one with no requestId -- anything getting past those resolves somebody else's export with the wrong bytes. happy-dom would really resolve and fetch the iframe src, so the suite depended on DNS and failed offline; disableIframePageLoading stops that. It signals the refusal by firing `error` on every iframe, which the script reads as a broken editor, so the harness holds that handler back and a load failure becomes something a test asks for. 188 lines: 50% -> 98.4%. What is left is a guard that cannot be reached once ensureIframe has resolved, and the DOMContentLoaded branch taken only when the script is parsed before the document is ready.
The script decorates the Media Library: it swaps .elpx thumbnails for scaled live previews, fills the details panel with metadata, and adds the "Edit in eXeLearning" and "Process as eXeLearning" buttons. At 239 lines and 0% coverage it was the largest untested file left. All of its markup is built by string concatenation and injected with jQuery .html()/.after(), which is why esc() exists and why four tests are about it: an attachment filename and .elpx metadata are attacker- controlled -- a contributor who can upload chooses both -- and they land in markup that runs in the reviewing administrator's session. Nothing asserted that escaping worked. The rest is idempotence and failure handling. The script is re-run by a MutationObserver, by the modal's `open` event and by four timers, so every function carries an "already did this" guard; a wrong guard means a duplicate button on every mouse move, so each one is exercised by running the updates twice. The reprocess call is covered end to end, including the 200-with-an-error-code case that resp.ok alone would read as success. The MutationObserver is stubbed rather than left real: a stale instance from a previous import would otherwise keep re-running updates during later tests, and a test needs to decide when a re-render happens. 239 lines: 0% -> 99.16%, functions 100%. The two remaining lines are guards whose only caller already returns before reaching them.
Two classes built their own path to dist/static/ instead of asking ExeLearning_Editor_Bundle, and nothing could point the plugin at a bundle of its own. Between them that left a block of code behaving differently depending on the machine: CI never runs make build-editor and .gitignore excludes dist/static/, so there the bundle is always absent, while a developer machine has a full editor build. Worse than the coverage hole, four tests branched on the environment instead of asserting a fixed outcome. AdminSettingsScreenTest and AdminSettingsTest each ran `if ( Editor_Bundle::is_available() )` and asserted something different per branch, and two ExportBootstrapPayload tests skipped each other out on the same condition -- one of them was the suite's only skipped test locally. A test whose assertion depends on whether someone ran make build-editor is not testing the plugin. class-styles-service.php and class-export-bootstrap.php now compose their paths from Editor_Bundle::get_path(), which yields the byte-identical string they built inline. The helper gains a path override that no-ops unless WP_TESTS_DOMAIN is defined, so tests supply a fixture bundle while a live site cannot relocate the editor. A filter would have been the obvious seam and is exactly what ADR-0002 rejects: it decides dist/static/ is the only runtime editor source, having turned down "let administrators supply the editor" for decoupling the served editor from the reviewed release. The four environment-branching tests are split into pairs that each assert one outcome against an explicit fixture, and the built-in styles table and theme manifest reader get real tests. admin/class-admin-settings.php 94.65% -> 100%, styles-service 94.87% -> 96.8%, export-bootstrap 81.81% -> 86.6%; suite 94.37% -> 94.59% with zero skips. That is 25 of the 120 bundle-gated lines. The other 95 need the two refactors SDD-0003 lists as follow-ups -- editor-bootstrap.php still tears down output buffering and maybe_render() still ends in exit -- and the fixture is their precondition, not a substitute. Designed in SDD-0003, linked from ADR-0002.
admin/views/editor-bootstrap.php builds the whole contract between WordPress and the embedded editor -- the REST endpoint and nonce the editor saves through, the <base> tag its assets resolve against, the approved style registry -- and then printed it and ended the process. None of it had a test, because merely running the file tore down every output buffer and called exit; EditorPageTest said so in its own header and settled for covering the guards in front of it. The E2E suite does not open the editor page either, so a nonce created for the wrong action or a <base> pointing at the wrong directory would have shipped unseen. PHP's include evaluates to whatever the included file returns, so the view stays where it is -- 450 lines of inline editor JavaScript belong in a template, not in a PHP class -- and becomes a function of its inputs. The buffer teardown, the headers and the echo move to the caller, and the "no bundle" case returns false instead of redirecting. ExeLearning_Editor grows build_bootstrap_page(), serve_bootstrap_page() and two protected one-liners holding what ends the process, following the ExeLearning_Admin_Styles::finish_request() pattern a test subclass overrides. maybe_render() gets the same split. The served bytes are unchanged: the view's diff touches only its docblock, the buffer loop, the bundle-missing branch and the tail, with the data preparation and every injection step untouched. One exception, and it is a fix. The pattern that finds the head element, /(<head[^>]*>)/i, also matches the editor's own <header id="head">, so a second stray <base> was injected into the middle of the body on every page. Browsers honour only the first, so nothing looked broken, but the markup was invalid. It now uses a word boundary and replaces once, verified against the real 125 KB bundle: one <base>, in the head, with <header id="head"> intact. editor-bootstrap.php 0% -> 90.8%, export-bootstrap 86.6% -> 90.6%, suite 94.59% -> 97.12%. What is left is the ABSPATH guard, a class_exists() fallback for a class always loaded, and the one-liners around exit. Designed in SDD-0004, building on SDD-0003.
The Gutenberg block was the last file with no tests at all: 141 lines at 0%, including the contract every saved post depends on and the wiring of the edit-mode download button. It is tested against the real @WordPress packages rather than hand-written stand-ins. @wordpress/element (React 18, the version WordPress ships), @wordpress/blocks, @wordpress/components and @wordpress/i18n are the same code that runs in the editor, so registerBlockType really validates the block, a real ToggleControl really renders its checkbox, and getByLabelText finds a control only when it was wired up accessibly -- a stub agrees with whatever the block passes it. Only wp.blockEditor is replaced, because it cannot be anything else: MediaUpload *is* the media modal, and BlockControls/InspectorControls are Slot/Fill pairs whose children render into slots the editor owns. Those pass-throughs substitute editor chrome, not the block's behaviour. Covered: the registered attributes (renaming one silently breaks every existing post), the closed-by-default settings, media selection including the plain-zip case WordPress reports on some servers, every inspector control, the download button reusing window.wpExeDownload so edit mode cannot drift from the frontend, the editorInstalled flag that wp_localize_script turns into '1'/'' , and the stylesheet the block injects into the same-origin preview to hide the teacher-mode toggler. Two tests were wrong before this landed. "Refuses to run a disabled format" passed because the DOM blocks clicks on a disabled button, not because the guard ran, so it would have passed with the wiring removed; it now asserts the mechanism. And the range-control test matched two elements, since RangeControl renders a slider and a number field. 141 lines: 0% -> 98.58%, JS suite 83.10% -> 95.93%. What is left is a guard no caller can reach and ResizableBox's onResizeStop. Costs 198 packages in the lockfile, npm ci measured at 31s.
Contributor
Test in WordPress PlaygroundTest the plugin with the code from this branch:
|
Rearranging admin/views/editor-bootstrap.php shifted the line numbers the POT and PO files record as source references, so the committed translations no longer matched what `wp i18n make-pot` produces and the CI consistency gate failed. Only the `#:` reference comments change -- 297 lines in, 297 out, across the POT and all ten shipped locales. No msgid or msgstr is touched, so nothing needs retranslating. Also excludes `artifacts` from the JavaScript sources validate-translations scans. It is gitignored and holds generated reports, but the PHPUnit HTML coverage report bundles its own JavaScript (d3.min.js and friends), so after any `make test-coverage` the next `make check-translations` failed locally with a wall of "Missing JSON for source artifacts/coverage/html/_js/..." for every locale. CI never saw it because CI has no artifacts/ directory when the gate runs; it just made the two documented targets impossible to run back to back.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
=============================================
+ Coverage 80.20% 96.99% +16.79%
- Complexity 848 862 +14
=============================================
Files 42 40 -2
Lines 4339 4357 +18
=============================================
+ Hits 3480 4226 +746
+ Misses 859 131 -728
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
erseco
added a commit
that referenced
this pull request
Aug 4, 2026
…iew (#89) * Stop the translation validator from reading directories Every run printed one "file_get_contents(): Read of N bytes failed with errno=21 Is a directory" notice per empty-of-JavaScript directory, in CI logs as well as locally. The filter callback returns true for a directory to mean "recurse into it", but RecursiveIteratorIterator defaults to LEAVES_ONLY, and a directory whose children the filter all rejected has no leaves -- so the directory itself is yielded as one. Skip directories in the loop. No behaviour change: those entries produced false from file_get_contents and were already skipped by the check below, just noisily. * Split the style upload validation out of handle_upload() PHPMD flagged handle_upload() at an NPath complexity of 1152 against a threshold of 500 (code scanning alert #36). The method is a flat chain of guards, but each one calls redirect_with_notice() rather than returning, and static analysis cannot know that ends the request -- so every guard counts as a branch that might fall through and the paths multiply. The four checks on the posted file move into accept_uploaded_archive(), which returns the validated paths or ends the request. handle_upload() is left with the capability check, the nonce, the install and the notice. $_FILES is still read in handle_upload() and passed in, rather than read in the callee. WordPress.Security.NonceVerification works per function scope, so reading the superglobal in a method that does not itself call check_admin_referer() trips the sniff -- and the honest fix is to keep the nonce check and the read together, not to suppress the warning. No behaviour change: the same checks run in the same order with the same messages. PHPMD reports no violation for the file now, PHPCS is clean, and the nine existing ExeLearning_Admin_Styles tests pass unchanged. * Move the block to Block API version 3 exelearning/elp-upload declared no apiVersion, so it registered as version 1, which WordPress 6.9 deprecates. That was not a local problem: a single API-version-1 block forces the whole post editor onto the non-iframe path, so the plugin was holding every editor it was installed in back from a change WordPress is completing. Three things stood in the way, and only the first is the one-line part. The version is now declared in both places -- apiVersion in the JS, api_version in register_block_type() -- with a test asserting they agree, since they sit far apart and disagreeing is silent. The visible output of both edit() branches is wrapped in one element carrying useBlockProps(), with the hook called before the early return; InspectorControls and BlockControls stay outside it, being Slot/Fill that render into the editor's own chrome. The stylesheets move onto the block type. Enqueued from enqueue_block_editor_assets they land in the outer admin document, which is not where an API version 3 block renders; declared as the block's style/editor_style, WordPress injects them into the canvas iframe. exelearning-frontend is still enqueued globally for the shortcode and deduplicates by handle. The fullscreen button is now wired by the component. elp-upload-fullscreen.js attached a click listener and a MutationObserver to the admin document and matched buttons to previews by DOM proximity -- none of which can see a block that lives in another document. The edit component already held a ref to the preview iframe, so the button gets a real onClick and a disabled state for a file with no preview. The script, its enqueue and its two test files are deleted: it existed only for the editor, and there is no version of it that can watch a document it is not in. The frontend button is unaffected; it is wired by inline JS emitted from PHP. The E2E test is what proves this. It failed first at `[data-type="exelearning/elp-upload"] .exelearning-block-preview iframe` resolving to 0 elements in the main document, which is exactly the migration working. It now looks inside iframe[name="editor-canvas"], toggles the real inspector control in the outer sidebar, and asserts the button appears in the canvas and takes the preview fullscreen. Its fullscreen stub moved from the element to the prototype: pinned to one node it vanished whenever the editor re-rendered, which made the assertion flaky in a full-suite run while passing in isolation. Designed in SDD-0005. * Pull dashicons into the canvas iframe with the block styles The download and fullscreen buttons rendered as blank boxes in the editor while looking correct on the published page. Moving the block to API version 3 in the previous commit moved its rendering into the editor canvas iframe, which receives the block's declared styles and their dependencies and nothing else. wp-admin loads dashicons into the outer document, which used to be where the block was; it is not any more. The frontend was unaffected because dashicons is enqueued there separately. Declaring dashicons as a dependency of exelearning-frontend is enough: WordPress then carries the font wherever that sheet goes, canvas included. The failure mode here is quiet -- no error, no missing file, just icons that do not draw -- so the E2E asserts the computed font-family of an icon inside the canvas rather than that a stylesheet link exists. Confirmed to fail without the dependency: the font falls back to the system sans-serif, which is exactly the blank box. * Refresh the translation references after the dashicons dependency The comment explaining why dashicons is a dependency shifted the line numbers that the POT and PO files record as source references. Only the `#:` comments change -- 55 lines in, 55 out, no msgid or msgstr touched.
erseco
added a commit
that referenced
this pull request
Aug 4, 2026
…-sandbox Brings in the coverage work (#88), the editor bootstrap refactor and the block's move to Block API version 3 (#89). Conflicts, and how they were taken: admin/views/editor-bootstrap.php -- both sides changed how the <base> tag is injected. This branch also injects a cache-purge script at the same point; main had fixed the pattern that finds the head element, because `<head[^>]*>` also matches the editor's own `<header id="head">`. Kept this branch's purge script with main's `\b` and the replace limit: without them the purge script was being injected, and run, twice. languages/* -- regenerated rather than hand-merged. main added no msgid, so this branch's translations carry over untouched and only the `#:` source references move. Tests that main added and this branch had already superseded: - vitest.config.mts: main disabled happy-dom iframe page loading for the whole suite, which breaks exe_embed.test.js -- that suite drives real iframes. Scoped the setting to wp_exe_download.test.js, the one file that needs it, with a @vitest-environment-options docblock. - elp_upload.test.js: the teacher-mode CSS-injection hack this branch replaced with `?exe-teacher=1` on the preview URL. Dropped; the replacement is already covered by elp_upload_preview.test.js. - exelearning_media_modal.test.js: the native-attachment-UI refactor removed the bespoke metadata panel, the "preview in new tab" link and the whole two-column actions row -- runAllUpdates() no longer calls addEditButtonToAttachmentInfo(). Those tests are gone. The details-panel ones are rewritten against what the panel does now: the preview replaces the thumbnail, and a single "Edit in eXeLearning" link sits below it carrying the class exelearning-editor.js binds to. - ContentProxyServeTest: one assertion compared the whole served document byte for byte, which no longer holds now that the proxy appends the embed shim. It asserts what it was about -- the absolute inline-style URL surviving untouched. 1007 PHP tests, 253 JS tests, PHPCS clean, translations deterministic.
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
2 participants
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.
Raises coverage from 80.2% to ~96.7% (PHP 94.37% → 97.12%, JavaScript 37.81% → 95.93%), in seven reviewable commits.
The starting point was a Codecov analysis of
main: 79% of the uncovered lines in the repository were JavaScript, and two thirds of the PHP gap sat behind a single condition — whether the machine happened to have a built editor indist/static/.What each commit does
exelearning.js,exelearning-admin.jswp-exe-bridge.jswp-exe-download.jsexelearning-media-modal.jsclass-admin-settings.phpeditor-bootstrap.phpelp-upload.jsTwo of these needed production changes, both designed up front:
SDD-0003 and
SDD-0004,
both linked to ADR-0002.
Beyond the number
Four tests used to branch on the environment.
AdminSettingsScreenTestandAdminSettingsTesteach ranif ( ExeLearning_Editor_Bundle::is_available() )and asserted something different per branch; two
ExportBootstrapPayloadTesttests skipped each other out on the same condition — one of them was the suite's
only skipped test on a developer machine. A test whose assertion depends on
whether someone ran
make build-editoris not testing the plugin. They are nowpairs that each assert one outcome against an explicit fixture bundle, and the
suite reports zero skips.
esc()in the media modal had nothing checking it. It is the only barrieragainst stored XSS from attachment filenames and
.elpxmetadata — both chosenby whoever uploads the file, both concatenated into markup injected with
.html()/.after(), and both running in the reviewing administrator's session.Four tests now cover it.
The editor page contract had no test at all. Everything the embedded editor
knows about WordPress — the REST endpoint, the
wp_restnonce, the<base>tagits assets resolve against, the approved style registry — is built by
editor-bootstrap.php, and merely running that file tore down PHPUnit's outputbuffering and called
exit. The E2E suite does not open the editor page either,so a nonce created for the wrong action would have shipped unnoticed.
Three real defects found on the way
A stray
<base>tag in every editor page. The pattern that finds the headelement,
/(<head[^>]*>)/i, also matches the editor's own<header id="head">, so a second<base href="…">was injected into themiddle of the body. Browsers honour only the first, so nothing looked broken,
but the markup was invalid. Fixed with a word boundary, verified against the
real 125 KB editor bundle.
The JS suite depended on DNS. happy-dom really resolved and fetched the
export iframe's
src, so the tests failed offline.disableIframePageLoadingstops that.
The block is registered with
apiVersion: 1, which WordPress 6.9deprecates: a single API-version-1 block makes the whole post editor fall
back to the non-iframe path. Not changed here — migrating to
apiVersion: 3means adopting
useBlockPropsand moving the editor styles fromenqueue_block_editor_assetsto the block type so they reach the iframedcanvas, and it has to be verified inside that canvas. It follows in its own
PR on top of this one.
Worth noting how it surfaced: no linter reported it. It appeared only once
the block's tests started registering it against the real
@wordpress/blockspackage instead of a stub.Two of my own tests were wrong, and the tests caught them
disabled button, not because the guard ran — it would have stayed green with
the wiring deleted. Spotted because the guard's line stayed uncovered.
renderButton()had already wiped the body, so it was true for the wrongreason. Spotted because
removeChildstayed uncovered.Both now assert the real mechanism. Coverage was useful here as a detector of
bad tests, which is most of why the per-line numbers were checked rather than
just the totals.
Testing approach
The Gutenberg block is tested against the real
@wordpresspackages —element(React 18, the version WordPress ships),blocks,componentsandi18n— not hand-written stand-ins.registerBlockTypereally validates theblock and a real
ToggleControlreally renders its checkbox, sogetByLabelTextfinds a control only when it was wired up accessibly. This iswhat caught the two bad tests above.
Only
wp.blockEditoris substituted, because it cannot be anything else:MediaUploadis the media modal, andBlockControls/InspectorControlsareSlot/Fill pairs whose children render into slots the editor owns.
Cost: 198 packages added to the lockfile;
npm cimeasured at 31s fromscratch, with the suite passing after a clean reinstall.
@wordpress/componentsis the bulk of it (36 MB). If that is too much for CI it can be dropped for
stubs — at the price of the real label and checked-state rendering that found
the bad tests.
A note for reviewers reading coverage in this repo
PHPUnit's
@coversdiscards everything executed outside the named class. Aline can report 0% while running on every test, attributed elsewhere. This bit
twice during the work: the bundle test seam, and
editor-bootstrap.phpstillshowing 0% with 13 green tests driving it.
EditorBootstrapPageTestthereforecarries no
@coversannotation on purpose — its subject is a view file, not aclass — and says so in its docblock.
Verification
make test— 940 tests, 2068 assertions, 0 skippednpm run test:js— 246 testsmake test-e2e— 40 tests, chromium + firefoxmake test-coverage— 97.12%, gateMIN_COVERAGE = 94phpcs --standard=.phpcs.xml.dist— cleanNot done
editor-bootstrap.phpkeeps itsABSPATHguard and aclass_exists()fallback uncovered; both are unreachable in a loaded plugin.
ExeLearning_Editor_Bundle::get_url()— three call sites still buildEXELEARNING_PLUGIN_URL . 'dist/static'by hand. Carried as a follow-up inboth SDDs.
ResizableBox'sonResizeStopin the block.