Test the UI's JavaScript, without adding a Node.js toolchain - #2435
Test the UI's JavaScript, without adding a Node.js toolchain#2435Flix6x wants to merge 10 commits into
Conversation
Context: - The modules under ui/static/js had no tests, and the only way we had checked them was by hand. They are plain ES modules, so they can be exercised directly, but ES imports do not work over file://. - A Node.js toolchain would be a heavy addition for ~5k lines of JavaScript, and would need its own CI job. Change: - Add a fixture that serves the modules over HTTP and runs a page of assertions in headless Chrome, reporting the results through pytest. Tests pass a snippet of JavaScript and assert on the checks it records. - Allow a test to place the browser in a chosen timezone, so that behaviour around daylight saving is not tied to the machine. - Add selenium to the test dependency group, which the runtime image does not install. The tests skip when selenium or Chrome is missing. Signed-off-by: F.N. Claessen <felix@seita.nl>
Change: - Test toIsoStringWithOffset, subtract and countDSTTransitions, the last under two fixed timezones, one with daylight saving and one without. - Test getUniqueValues and convertToCSV. - Test decompressChartData, including the pass-through of data already in the old format and the conversion of seconds-valued sensors to dates. Signed-off-by: F.N. Claessen <felix@seita.nl>
Context: - toIsoStringWithOffset appended the local UTC offset to date.toISOString(), which is UTC, without moving the clock time. The result named an instant wrong by exactly that offset. Its only caller sends the KPI window to the API, so KPIs could cover the wrong days, by a whole day west of UTC. - getUniqueValues ended its loop at the first falsy entry, and tested for values already seen with `in`, which consults the prototype chain, so a source named "constructor" or "toString" was silently dropped. Change: - Write the local clock time, then append the offset. - Iterate by index, track what has been seen in a Set, and guard the nested lookup against a missing record. Signed-off-by: F.N. Claessen <felix@seita.nl>
…ge fix Context: - One is infrastructure for plugin developers and hosts, the other is a user-visible fix. Change: - Add both to v1.1.0. PR numbers are placeholders until the PR exists. Signed-off-by: F.N. Claessen <felix@seita.nl>
Context: - The entries were added before the PR existed, with XXXX placeholders. Change: - Point both at PR #2435. Signed-off-by: F.N. Claessen <felix@seita.nl>
There was a problem hiding this comment.
Pull request overview
This PR adds a pytest-driven harness to execute the UI’s ES-module JavaScript in a real headless Chrome session (without introducing a Node.js toolchain), and uses it to add initial coverage for key date/data/chart helpers while fixing two UI bugs uncovered by those tests.
Changes:
- Add a selenium-based pytest fixture that serves
ui/static/jsover HTTP and runs JS assertions in headless Chrome, with optional timezone override via CDP. - Introduce a small JS test suite covering
daterange-utils,data-utils, andchart-data-utils. - Fix
toIsoStringWithOffset(preserve the represented instant) and hardengetUniqueValues(handle falsy entries and prototype-colliding keys), plus add changelog entries.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds selenium to the test dependency group for the new JS test harness |
| flexmeasures/ui/tests/js/conftest.py | Implements the HTTP-serving + headless-Chrome JS runner and assert_js helper |
| flexmeasures/ui/tests/js/README.md | Documents how the JS tests work and how to run them |
| flexmeasures/ui/tests/js/test_daterange_utils.py | Adds browser-level tests for date formatting and DST counting |
| flexmeasures/ui/tests/js/test_data_utils.py | Adds tests for getUniqueValues edge cases and CSV conversion |
| flexmeasures/ui/tests/js/test_chart_data_utils.py | Adds tests for decompressing chart data formats |
| flexmeasures/ui/static/js/daterange-utils.js | Fixes toIsoStringWithOffset to write local time with offset without changing the instant |
| flexmeasures/ui/static/js/data-utils.js | Reworks getUniqueValues to avoid falsy-loop termination and prototype-chain collisions |
| documentation/changelog.rst | Adds changelog entries for the new JS test capability and the KPI time-range bugfix |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const val = getValueByNestedKey(data[i], key); | ||
|
|
||
| if (!(val in lookup)) { | ||
| lookup[val] = 1; | ||
| if (!seen.has(val)) { | ||
| seen.add(val); | ||
| results.push(val); |
There was a problem hiding this comment.
Good catch, fixed. And it reaches further than the null-entry change that prompted it: a record carrying no source at all already yielded an undefined into the results, before this PR. So checkSourceMasking could warn that "only data from the most prevalent source is shown" when there was only ever one real source and one record without one.
getUniqueValues now skips absent values, with a test asserting that a record without a source contributes nothing, and that a list of only such records reports none.
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
Documentation build overview
10 files changed ·
|
Context: - Review feedback on getUniqueValues: a record that lacks the key yields undefined, which then landed in the results. - Its only caller, checkSourceMasking, counts distinct source ids to decide whether to warn that data is being masked, so one record without a source made it warn about masking that was not happening. - This predates the null-entry fix in this PR: a record carrying no source at all already contributed an undefined. Change: - Skip values that are absent, and assert that a record without a source contributes nothing. Signed-off-by: F.N. Claessen <felix@seita.nl>
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
Context: - CI installs from uv.lock with --frozen, so adding selenium to the test group without relocking failed the pre-commit check and the image build. - Locking with an older uv rewrote the file to an earlier revision, so this was done with 0.10.9, the version CI pins. Change: - Add selenium and its eight dependencies. Nothing else moved: no package was removed and no existing version changed. - Apply black to the new test fixture. Signed-off-by: F.N. Claessen <felix@seita.nl>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Context: - CI failed on formatting the new test files. The repo pins black 26.3.1, which hugs multiline strings; the black in my environment was 25.1.0, which leaves them wrapped, so it reported the files as clean. Change: - Reformat with 26.3.1. Signed-off-by: F.N. Claessen <felix@seita.nl>
|
CI is green, and worth confirming explicitly: the JavaScript tests run on the CI runners, they are not silently skipped. From the 3.11 job log: So Two CI failures on the way here, both mine, both worth recording for anyone adding a dependency:
|
Context: - toIsoStringWithOffset appended the local UTC offset to date.toISOString(), which is UTC, without moving the clock time, so the string named an instant wrong by exactly that offset. - getAssetKPIs is its only caller, so this shifted the very window this PR is about. West of UTC the shift lands on a different day for sensors of daily resolution, so KPIs could report a different day than the chart. Change: - Write the local clock time, then append the offset. - The same fix is in PR #2435, where the JavaScript tests that found it live. Both branches carry identical text, so they merge either way. The changelog entry for it stays in #2435. Signed-off-by: F.N. Claessen <felix@seita.nl>
|
Filed #2436 for the other half of this: the ~1,000 lines of JavaScript still inside |
| """Run a snippet, optionally pretending the browser sits in a given timezone. | ||
|
|
||
| Overriding the timezone keeps tests that depend on one, such as daylight saving | ||
| transitions, independent of the machine running them. | ||
| """ |
There was a problem hiding this comment.
Fixed in 7d22b41 — reflowed so each line ends at a comma or a period.
I swept the rest of this PR's Python for the same thing rather than fixing only the line you named. The only other candidates were embedded HTML and JavaScript inside triple-quoted strings, which is code rather than prose, and # noqa / # pragma directives. So this was the one.
Context: - Review feedback: the sentence wrapped after "daylight saving", against the repo convention, which PR #2433 has just extended to cover JavaScript as well as Python. Change: - Reflow so each line ends at a comma or a period. Signed-off-by: F.N. Claessen <felix@seita.nl>
Description
The UI's JavaScript had no tests. This adds a way to run it, and fixes the two bugs that doing so immediately found.
ui/tests: run the modules underui/static/jsin headless Chrome, driven from pytestui/tests: cover the date, data and chart-data helpersui/static: fixtoIsoStringWithOffsetandgetUniqueValuesdocumentation/changelog.rstNo Node.js
The modules are plain ES modules, so they can be exercised directly. A fixture serves them over HTTP — ES imports do not work over
file://— and runs a page of assertions in headless Chrome. A test passes a snippet of JavaScript that imports what it needs and callscheck(...)oreq(...); pytest reports whatever did not pass.Deliberately no JavaScript framework: no
package.json, no bundler, nonode_modules, no second CI job. The only new dependency isselenium, in thetestdependency group, which the runtime image does not install (uv syncin the Dockerfile takesdevonly), so the image does not grow. Tests skip when selenium or Chrome is missing, so a checkout without either still runs the Python suite.ubuntu-latestships Chrome, so CI needs no extra setup.A test can place the browser in a chosen timezone through CDP, so behaviour around daylight saving is pinned rather than inherited from whatever machine runs it. The suite passes under
TZ=UTC,America/New_York,Europe/AmsterdamandPacific/Auckland.11 tests, ~9 s including browser startup.
Look & Feel
Two real bugs, both found by writing the first tests.
toIsoStringWithOffsetnamed the wrong instant. It appended the local UTC offset todate.toISOString(), which is UTC, without moving the clock time:Those name instants two hours apart. The result is wrong by exactly the viewer's offset, in the direction of the offset.
Its only caller is
getAssetKPIs, so the time range sent when loading an asset's KPIs was shifted. East of UTC the window moves earlier, west of UTC later — and since the shift shows up as a whole-day change for daily KPI sensors west of UTC, KPIs there could report a different day's data than the chart beside them.getUniqueValuesdropped values. It ended its loop at the first falsy entry (for (var item, i = 0; item = items[i++];)) and tested for values already seen within, which consults the prototype chain. A data source namedconstructorortoString— source names are user data — was silently discarded:How to test
Revert either fix and the corresponding test fails, naming the instant or the missing value.
Notes
Related to #2434, which fixes a different bug in the same KPI time range: that one sends an end date a day too late, this one sends both ends shifted by the viewer's offset. The two are independent — different files, no conflict — and each is wrong on its own. Both change reported KPI values, so they are worth reviewing together even though they merge separately.
Also worth flagging: the tooling here reaches
.jsfiles only. Around 1,000 lines of JavaScript still live insidegraphs.html, where no formatter or linter can reach them, because the Jinja tags break every JavaScript parser. Moving that code into modules is what would bring it under test.