Add unit tests for admin, build-log, measure and translation controllers (#686)#693
Merged
Merged
Conversation
…ers (#686) Brings the four under-covered controllers up to the testing-initiative target, exercising both happy and unhappy paths and the authorization / validation guards on each: - admin.ts: 23% -> 97.5% statements - build-log.ts: 34% -> 100% statements - measure.ts: 20% -> 92.3% statements - translation.ts: 22% -> 92.1% statements Three tests are intentionally left FAILING because they document real defects found while writing the tests (see PR description). They assert the correct behaviour and are expected to go red until the bugs are fixed: 1. admin.loadUserGroup / admin.loadUser call next() twice on the not-found path (missing return in the catch block), unlike the canonical datasetAuth middleware which returns after next(error). 2. measure.updateMeasureMetadata is missing a return after next(NotFoundException) when the dataset has no measure, so it falls through and dereferences the null measure. Tests only - no source changes.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Jest unit tests for four controllers (admin, build-log, measure, translation) flagged as under-covered by #686, raising each well above the 70% statement-coverage target. No source changes are included. Three tests are intentionally left red to document two real next()-after-guard bugs (admin.loadUserGroup/loadUser and measure.updateMeasureMetadata), per the PR author's explicit design choice.
Changes:
- New test file
test/unit/controllers/admin.test.tsexercising group/user CRUD, role/status validation, dashboard, similar-dataset reports, and search-log export — including two intentionally-failing tests for the double-next()bug inloadUserGroup/loadUser. - New test file
test/unit/controllers/measure.test.tscovering measure info/preview/reset/metadata-update and lookup-table attach/download paths — including one intentionally-failing test for the missingreturnafter the no-measure guard inupdateMeasureMetadata. - New test files
test/unit/controllers/build-log.test.tsandtest/unit/controllers/translation.test.tscovering paging/validation, CSV streaming, av-scan failures, and cube-rebuild failures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/unit/controllers/admin.test.ts |
Full controller coverage; includes 2 red tests asserting single next() invocation in loadUserGroup / loadUser. |
test/unit/controllers/measure.test.ts |
Covers CRUD + lookup-table flows; includes 1 red test asserting updateMeasureMetadata returns after the no-measure NotFoundException. |
test/unit/controllers/build-log.test.ts |
Default/parameterised paging, type/status validation errors, and not-found paths for getBuiltLogEntry. |
test/unit/controllers/translation.test.ts |
Preview, export, validate-import (row-count and key mismatches), and apply-import including cube-rebuild failure. |
…trollers Add the missing `return` after next(error) in three controller guards that the new tests surfaced: - admin.loadUserGroup / admin.loadUser: the catch block fell through to the unconditional next() at the end of the function, calling next() twice on the not-found path. Return after next(error), matching the canonical datasetAuth middleware. - measure.updateMeasureMetadata: the missing-measure guard fell through and dereferenced the null measure (TypeError). Return after next(NotFoundException), matching getPreviewOfMeasure / getMeasureInfo. The three tests added in the previous commit now pass and serve as regression guards.
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.
Summary
Adds unit tests for the four under-covered controllers flagged in #686, bringing each well above the 70% statement-coverage target, and fixes two bugs the tests surfaced.
src/controllers/admin.tssrc/controllers/build-log.tssrc/controllers/measure.tssrc/controllers/translation.tsBoth happy and unhappy paths are exercised, including the authorization / validation guards (uuid validation, status validation,
byvalidation, unique-constraint handling, av-scan failures, missing-measure / missing-revision guards).Bugs found and fixed
Writing the tests surfaced two real defects, each fixed here with a one-line
return. The tests that caught them now pass and act as regression guards.Bug 1 —
admin.tsloadUserGroupandloadUsercallednext()twice on the not-found pathWithout the
returnthe catch fell through to the unconditionalnext()at the end of the function, invokingnexttwice (once with the error, once without) and letting the request continue to the route handler with nothing loaded inres.locals— the same "responds twice" class of bug fixed in95adb9db. Now matches the canonicaldatasetAuthmiddleware (src/middleware/dataset-auth.ts).Bug 2 —
measure.tsupdateMeasureMetadatafell through its missing-measure guardWithout the
return, execution dereferenced the nullmeasureand threw aTypeErrorinstead of returning a clean 404. Now matchesgetPreviewOfMeasure/getMeasureInfo.Regression tests:
admin.loadUserGroup › calls next exactly once with NotFound when the group cannot be loadedadmin.loadUser › calls next exactly once with NotFound when the user cannot be loadedmeasure.updateMeasureMetadata › forwards NotFound and does not throw when the dataset has no measureScope
This PR covers the four controllers from #686. The three services (
dataset,revision,task) are handled separately in #692. Thejest.config.tscoverage thresholds are left unchanged here; raising them is best done once both PRs land.Verification
jest --coverage(figures above).eslintandtsc --noEmitclean.Part of #686.