Feature/add testing - #1732
Open
duderoot wants to merge 3 commits into
Open
Conversation
The project had no test runner, so behaviour could only be verified by hand against a running instance. Add Jest with jsdom and @vue/test-utils, exposed as `npm test`, `npm run test-watch` and `npm run test-coverage`. Specs live in `tests/`. Vue SFC compilation is deliberately left out: the code under test is plain JavaScript (mixins and shared modules), so the setup avoids depending on vue-jest. Generated coverage output is excluded from ESLint and Prettier, which would otherwise lint the report. Signed-off-by: duderoot <catalin.patruica@xlab-iq.de> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Jest setup covered plain JavaScript only, so component behaviour could not be asserted. Add @vue/vue2-jest so `.vue` files are transformed, and map stylesheet imports to a stub, since Jest cannot parse CSS and styles carry no behaviour worth asserting. Signed-off-by: duderoot <catalin.patruica@xlab-iq.de> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Jest harness added earlier on this branch had no specs. This adds the scaffolding a real suite needs and the first 405 tests, covering src/shared, the permissions and filter pill mixins, and three of the filter pill components. Config changes needed to make the harness usable: - flexver ships ESM at flexver/dist/module, so it and lodash-es are exempted from transformIgnorePatterns rather than remapped to a CJS build the app does not ship. - @/i18n is mapped to a test double; the real module calls webpack's require.context and fetches the default locale at import time. - vue is mapped to the full build, since the package entrypoint is runtime-only and test hosts and stubs use string templates. - src is added to roots so files no spec imports are still reported as uncovered. Without it coverage read 89.97% over 14 files instead of 50.28% over 32. Coverage thresholds are the measured values rounded down. permissions.js and utils.js get their own high floors because they gate authorization and guard against open redirects; everything else stays in the global pool so the untested mixins and pills remain visible. Some tests pin current behaviour rather than ideal behaviour, each with a comment: capitalize() ignores strings of two characters or fewer, isUrlSaveForRedirect() matches by prefix rather than by path segment, and hasPermission() returns undefined for a non-string, non-array argument. Two things worth a follow-up, both left alone here to keep this test-only: makeAnalysisResponseLabelFormatter is defined in shared/common.js but missing from its hand-maintained default export, so it is unreachable; and permissionsMixin's PERMISSIONS map is asserted against the constants the shared module exports rather than a fixed count, so the two cannot drift apart unnoticed. Unit tests now run as a blocking job in the lint workflow. Signed-off-by: duderoot <catalin.patruica@xlab-iq.de> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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.
Description
Adds a unit test suite for the frontend. The Jest harness introduced earlier on this branch had no specs, so
npm testexited non-zero with "No tests found". This fills it in with 405 tests across 12 suites coveringsrc/shared/, the permissions and filter pill mixins, and three filter pill components, and wires the suite into CI as a blocking job.No production code under
src/is modified — the change is limited to tests, Jest configuration, the CI workflow, and developer documentation.Addressed Issue
##1731
Additional Details
Configuration changes required to make the harness usable
The existing config could not actually load the modules under test. Four fixes were needed:
transformIgnorePatternsnow exemptsflexverandlodash-es.src/shared/utils.jsimportsflexver/dist/module, which is ESM, so Jest's defaultnode_modulesexclusion made it fail to parse. The alternative — remapping to the CJSflexver/dist/nodebuild — was rejected becausecompareVersionswould then be tested against a build the app does not ship.@/i18nis mapped to a test double. The real module calls webpack'srequire.contextand issues an HTTP request for the default locale at import time, neither of which works outside the webpack build. This is done viamoduleNameMapperrather than a__mocks__directory because the import is transitive (utils.js→@/i18n), which is exactly where a per-filejest.mock()gets forgotten.vueis mapped to the full build. The package entrypoint isdist/vue.runtime.common.js, so string templates in test host components and stubs would not compile.srcwas added toroots. Without it, files that no spec imports were invisible to the coverage collector, and coverage reported 89.97 % over 14 files. The honest figure across all 32 files matched bycollectCoverageFromis 50.28 %.Coverage thresholds
Thresholds are the measured values rounded down, not aspirational targets. Because Jest removes any file matched by a path key from the
globalbucket, per-file carve-outs are limited to the two files where a silent regression is a security problem:shared/permissions.js(authorization gating, 100 %) andshared/utils.js(theisUrlSaveForRedirectopen-redirect guard, 95/90/95/95). Everything else stays in the global pool, so the eight mixins and six filter pills that still have no specs remain visible as debt rather than hidden behind a narrowedcollectCoverageFrom.Tests that pin current behaviour
Several assertions document quirks rather than assert ideal behaviour, each with a comment explaining why, so that changing them later is a deliberate and visible decision:
common.capitalize()leaves strings of two characters or fewer untouched (capitalize('ab') === 'ab'). This leaks intotitleCase('a-b') === 'a b'and into the storage key built byloadUserPreferencesForBootstrapTablefor a two-character column (myTableShowid, notmyTableShowId).isUrlSaveForRedirect()matches by prefix rather than by path segment, so/projectsomethingis accepted.hasPermission()returnsundefinedfor an argument that is neither a string nor an array — there is noelsebranch.formatSeverityLabel()returns''for falsy input while its sibling formatters returnnull.Two findings surfaced by writing the tests
Both were left alone to keep this change test-only, and are worth separate PRs:
makeAnalysisResponseLabelFormatteris defined insrc/shared/common.jsbut is missing from that module's hand-maintained default export, so it is unreachable from the public API. An export-surface test locks the current key set and documents the omission.permissionsMixin'sPERMISSIONSmap is maintained by hand alongside the constants inshared/permissions.js. The test now asserts the map against the constants the shared module actually exports rather than a hardcoded count, so the two cannot drift apart unnoticed.Checklist
[ ] This PR introduces new or alters existing behavior, and I have updated the documentation accordingly