test(js): vitest + jsdom unit tests for scheming-ai-suggestions.js (+ fixes 1 polling bug)#304
Conversation
The 419-line scheming-ai-suggestions.js polling controller is the
JS half of the AI-suggestions feature contract: the Python
``AISuggestionsStage`` writes ``package["dpp_suggestions"]
["ai_suggestions"]``, this JS reads it via ``package_show`` polling
and surfaces per-field buttons. Before this PR there was zero JS
test coverage — manual UI checks were the only validation.
Adds:
* ``package.json`` + ``vitest.config.js`` — minimal Vitest + jsdom
+ jquery devDependencies (~24 packages, no Babel toolchain).
* ``tests/js/setup.js`` — loads jQuery directly into the jsdom
realm (the standard ``jquery(window)`` returns the jQuery
namespace object under vitest's ESM-CJS interop, NOT the
callable ``$`` selector). Stubs ``ckan.module`` to capture the
module registration. Provides ``buildInstance`` + ``stubAjax``
helpers so tests can drive the polling state machine with
``vi.useFakeTimers()``.
* ``tests/js/scheming-ai-suggestions.test.js`` — 12 tests across
4 describe blocks:
- Module registration (3): name, options defaults, exposed methods.
- ``initialize()`` early-return guard (2): bails when
``data-field-name`` is missing (the guard added in PR #302's
Copilot-review follow-up); hides + starts polling when
present.
- ``_pollForAiSuggestions()`` state machine (4): STATUS=DONE
termination, polls again when ai_suggestions absent, caps at
maxPollAttempts, retries with exponential backoff on ajax
error.
- ``_showAiSuggestionButtons()`` DOM updates (3): reveals +
sets ``data-suggestion-value`` / ``data-suggestion-source``;
leaves unmatched buttons hidden; no-ops on empty value.
* ``tests/js/README.md`` — how to run, what's covered, what's
deferred (popover interactions; real CKAN-on-page integration),
and how the harness works.
* ``.gitignore`` — node_modules / package-lock.json / coverage.
The harness immediately caught two real bugs:
1. **JS polling bug** (the most important finding):
``scheming-ai-suggestions.js`` was reading ``STATUS`` from
``dpp_suggestions.STATUS`` (top-level), but
``AISuggestionsStage._reshape_for_ui`` writes it INSIDE
``ai_suggestions.STATUS``. The terminal status check therefore
never fires — polling runs out the 100s ``maxPollAttempts``
clock on every successful AI suggestion run, burning ~40
redundant ``package_show`` calls. Fixed: JS now reads
``dpp_suggestions.ai_suggestions.STATUS`` first, falls back
to the top-level value for forward-compat with custom plugins
that mirror the legacy convention.
2. **jQuery deprecation cleanup**:
``$(document).ready(handler)`` was deprecated in jQuery 3.0
in favour of ``$(handler)``. Same behavior, cleaner. Removes
the TypeScript-style deprecation warning the linter was
flagging.
Test summary: 12 passed. Run via ``npm test``. Full Python suite
(171 tests) unaffected — JS lives in its own toolchain.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a minimal JavaScript unit-test toolchain (Vitest + jsdom + jQuery) to cover the scheming-ai-suggestions.js polling controller and fixes a production polling termination bug by reading STATUS from dpp_suggestions.ai_suggestions.STATUS.
Changes:
- Introduces Vitest/jsdom-based JS test harness under
tests/js/plus a rootvitest.config.js. - Adds 12 unit tests covering module registration,
initialize()guard behavior, polling state machine behavior, and DOM updates. - Fixes polling termination by reading
STATUSfrom the correct location and updates the jQuery ready handler idiom.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.js |
Adds Vitest configuration to run only tests/js/** under jsdom with a global setup file. |
package.json |
Introduces a small npm devDependency set and test scripts for running the JS unit suite. |
tests/js/setup.js |
Implements the jsdom/jQuery/CKAN-module stubs and helpers to load the side-effect SUT and stub ajax. |
tests/js/scheming-ai-suggestions.test.js |
Adds unit coverage for initialization and polling logic of scheming-ai-suggestions.js. |
tests/js/README.md |
Documents how to run the JS tests and what contract they cover. |
ckanext/datapusher_plus/assets/js/scheming-ai-suggestions.js |
Fixes STATUS lookup for polling termination and updates document-ready binding. |
.gitignore |
Ignores node/vitest artifacts (node_modules, coverage, package-lock). |
Comments suppressed due to low confidence (2)
tests/js/setup.js:77
stubAjax()overwriteswindow.$.ajaxbut nothing restores the original implementation, and the globalconst $ = window.$is reused across tests. This can cause cross-test bleed (a later test may accidentally hit a stubbed ajax from a previous test). Capture the original$.ajaxonce and restore it in the globalbeforeEach/afterEach, or havestubAjaxreturn a restore function that tests call inafterEach.
beforeEach(() => {
// Reset DOM + globals between tests so module state doesn't bleed.
document.body.innerHTML = '';
document.documentElement.innerHTML = '<head></head><body></body>';
window.$ = $;
window.jQuery = $;
global.$ = $;
global.jQuery = $;
// CKAN module registrar stub. The real ckan.module(name, callback)
// wires the returned object up for DOM auto-discovery via
// ``[data-module=name]``. For tests we just capture the callback
// so we can instantiate the returned object manually with the
// ``this`` binding (``this.el``, ``this.options``) we choose.
window.ckan = {
SITE_ROOT: '',
module: (name, factory) => {
registeredFactory = { name, factory };
},
};
global.ckan = window.ckan;
// Wipe any leaked global state from prior tests.
delete window._schemingAiSuggestionsGlobalState;
// Wipe any previously-registered factory.
registeredFactory = null;
});
tests/js/scheming-ai-suggestions.test.js:111
- Test name mentions
dpp_suggestions.STATUS, but the asserted terminal status in the fixture isdpp_suggestions.ai_suggestions.STATUS(matching the production fix). Rename the test to reflect the actual contract so future readers don’t reintroduce the original bug.
it('stops polling when dpp_suggestions.STATUS is in terminalStatuses', () => {
// Seed a button + datasetId; one ajax response with STATUS=DONE.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Five findings, all stylistic/portability — applied verbatim.
* setup.js: ``__dirname`` was relying on Vitest's CommonJS compat
shim (works under Vitest, would break under any ESM-native
runner). Switched to ``path.dirname(fileURLToPath(import.meta.url))``
— the proper ESM idiom, runner-agnostic.
* scheming-ai-suggestions.test.js: replaced
``Object.defineProperty(window, 'location', ...)`` (fragile in
strict jsdom configs where ``location`` is non-configurable)
with ``window.history.pushState({}, '', '/dataset/edit/<id>')`` —
the browser-equivalent idiom that survives any jsdom version.
* scheming-ai-suggestions.test.js: reworded the misleading
"no global state touched" comment. The script DOES initialize
``window._schemingAiSuggestionsGlobalState`` at top-level load
time — what the guard prevents is ``initialize()`` going on to
*mutate* that already-initialized state for elements with no
``data-field-name``.
* setup.js: ``stubAjax`` docstring example showed ``STATUS`` at
the wrong level (top-level ``dpp_suggestions``); updated to
``dpp_suggestions.ai_suggestions.STATUS`` matching the contract
this PR's production fix established.
* vitest.config.js: header comment said the SUT was loaded via
``eval``, but setup.js actually uses ``new Function(...)``.
Updated to match — an earlier draft did use ``eval`` and the
comment drifted when I switched to ``new Function`` for the
``\$``-in-scope reason.
Tests: 12/12 still pass — pure cleanup, no behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five LOW findings, all applied.
* test.js (DOM, "leaves unrelated buttons hidden"): replaced the
manual ``inst = {el, options: factory().options, ...factory()}``
construction. The options spread was immediately overwritten by
the trailing ``...factory()`` spread (silently dropped), and the
factory was called twice yielding two unrelated instances. Just
uses ``buildInstance`` like the other tests now.
* test.js ("keeps polling when ai_suggestions is absent"): added a
final ``advanceTimersByTime(2500); expect(calls.length).toBe(2)``
assertion. Without it, a regression that ignored terminal STATUS
would still pass — the first response would re-trigger polling
and the count would hit 2 regardless of whether DONE actually
stopped the loop.
* setup.js (``stubAjax``): now throws on unexpected extra calls
(queue exhausted) instead of silently returning. Caught a real
test gap on the very next run — the "hides element AND starts
polling" test was passing without queueing a response for the
polling call initialize() triggers; silently swallowed before,
now surfaces immediately. Fixed by adding a sentinel ``{}``
response in that test (documented in the new stubAjax docstring
as the escape hatch for tests that want to count calls but
don't care about responses).
* .gitignore + package-lock.json: removed package-lock.json from
.gitignore and committed it. For a private test harness (no
``"private": true`` lie — the package.json already declares
it) we want dev + CI to install identical transitive versions
of jsdom / vitest / jquery so test behavior is reproducible.
Standard npm practice for apps and test harnesses.
* scheming-ai-suggestions.js: switched the STATUS coalesce from
``||`` to a nullish check. An empty-string ``ai_suggestions.STATUS``
(unlikely but possible upstream state — e.g. a half-written
package extra) was silently falling through to the legacy
top-level STATUS via ``||``. Treating empty string as "no
status, keep polling" is the right behavior — switched to an
explicit ``!== undefined && !== null`` check (functionally
``??`` but more explicit for an old-browser ES target).
Tests: 12/12 still pass after the strict-stub fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Took a pass through the two low-confidence comments from review #4305403052: 1. 2. Test name says Both pushed in commit 2df3ebd. 12/12 tests still pass. |
Two suppressed-as-low-confidence comments from review #4305403052, both legitimate. * setup.js: ``stubAjax`` overwrote ``window.$.ajax`` without ever restoring it. Since ``window.$`` is the same jQuery instance across every test (captured once at module load), the stub silently leaked into the next test — a subsequent test that didn't call ``stubAjax`` would still see the previous test's stub. Captured ``originalAjax = $.ajax`` at module load and restore it in the ``beforeEach``. Tests that want the stub now have to explicitly call ``stubAjax``; default state matches a fresh page load. * scheming-ai-suggestions.test.js: renamed the polling-termination test from ``stops polling when dpp_suggestions.STATUS is in terminalStatuses`` to ``stops polling when dpp_suggestions.ai_suggestions.STATUS is in terminalStatuses`` — matches the actual contract this PR established (and the fixture the test asserts on). The old name would invite a future reader to "fix" the code back to the pre-PR-#304 bug thinking the test was pinning the wrong path. Tests: 12/12 still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Adds the missing JS test infrastructure for the 419-line scheming-ai-suggestions.js polling controller. The harness immediately caught a real polling bug (
STATUSread from the wrong key) — fixed in the same PR. The same pattern as PR #303 catching real qsv bugs once E2E was actually exercised: feature surface that wasn't tested was, predictably, broken.What's added
package.jsonvitest.config.jstests/js/setup.jsckan.modulestub;buildInstance+stubAjaxhelperstests/js/scheming-ai-suggestions.test.jstests/js/README.md.gitignoreCoverage (12 tests)
initialize()early-return guard (2) — bails whendata-field-nameis missing (the guard added in feat(ai-suggestions): scheming UI for AI-derived metadata (PR #253 follow-up) #302's Copilot follow-up); hides + starts polling when present_pollForAiSuggestions()state machine (4) — STATUS=DONE termination, polls again when ai_suggestions absent, caps atmaxPollAttempts, retries with exponential backoff on ajax error_showAiSuggestionButtons()DOM updates (3) — reveals + sets data attrs; leaves unmatched buttons hidden; no-ops on empty valueReal bug found
Polling never terminated in production. The JS was reading
STATUSfromdpp_suggestions.STATUS(top level), butAISuggestionsStage._reshape_for_uiwrites it INSIDEai_suggestions.STATUS. The terminal status check never fired → polling ran out the 100smaxPollAttemptsclock on every successful AI suggestion run, burning ~40 redundantpackage_showcalls per dataset edit.Fix: JS now reads
dpp_suggestions.ai_suggestions.STATUSfirst, falls back to the top-level value for forward-compat with custom plugins that mirror the legacy convention. Same blast pattern as the qsv bugs caught by E2E in #303: untested surface == broken surface.Cleanup
$(document).ready(handler)→$(handler)(jQuery 3.0+ idiom; clears the deprecation warning).Run
Node 22+ recommended. The Python test suite (171 tests) is unaffected — JS lives in its own toolchain.
What's NOT covered (deferred)
Why Vitest (not Jest / Karma)
Vitest + jsdom is the minimum-footprint choice for a Python-first project. ~24 npm packages total vs ~500 for Jest+jsdom+Babel. Single
vitest.config.js; no Babel toolchain. Documented intests/js/README.md.🤖 Generated with Claude Code