test(download): target the approve button and the viewer options dropdown #413
Assignees
allan (Allan)
dcbot
DylanLeong
henrik
hermes (hermes)
mihajlo
sabir
sead
trevor
Yury
Clear assignees
allan (Allan)
dcbot
DylanLeong
henrik
hermes (hermes)
mihajlo
sabir
sead
trevor
Yury
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: dc/dc#413
Reference in new issue
Block a user
What
download.cy.tscannot pass as written, and the reason is not an overlay covering the button - it is a dead route plus two selectors that no longer exist. The spec is also back in the CI list, so this PR is where it runs.The first CI run of this branch (7 failures, everything else green: 166 passing, 1 spec failing) is what pinned it down. My first commit in this branch guessed at the cause from the issue text and the error message; it was wrong about test 1, and this commit replaces it.
Finding 1 -
#/approve/toapproveis not a routeThe review module is namespaced:
app.routing.tsmounts it atreview, andreview-routing.module.tsdeclaresapprove,approveDet/:tableId,submitted,history. So the approve screen is#/review/approve, and#/approve/toapproverenders the router's 404 page. The original.btn.btn-successassertion matched a success button on that 404/welcome page - abtn btn-sm btn-success, not the approve row'sbtn btn-success- which is why the error read like an element hidden under a fixed container.Confirmed by opening both URLs on a local estate:
#/approve/toapproverenders "404 - Not Found",#/review/approverenders the approve screen.Finding 2 - a seeded estate has no approvals to download
Even on the right route the grid is empty:
The grid draws one download button per table that is waiting for approval (
approve.component.htmlrenders it inside@if (approveList && remained !== 0)), andmpe_makedataseeds no submissions. Test 1 now creates its own: it uploadsexcels/regular_excel.xlsxtompe_x_testand submits it from the stage screen, using the same two helpersstage.cy.tsuses, then downloads the audit file for the table it just submitted. That also removes the ordering dependency on whatever earlier specs leave behind.Finding 3 - the viewer's options menu is not a list of buttons
Every entry in that menu is a
divcarryingclrDropdownItem- Download, Filter, Web Query URL, Lineage (viewer.component.html) - so abuttonselector can never match insideclr-dropdown-menu.filter-panel.cy.tsalready opens Filter from the same menu withcy.get('clr-dropdown-menu').contains('Filter'), and the helper now does the same for Download. Thedropdown-toggleclass it also required is not on that element either.CI list
downloadmoves fromEXCLUDEDto the end ofSPECSin.gitea/scripts/cypress-specs.sh, appended rather than inserted. Coverage check:All 24 specs are accounted for: 23 run, 1 excluded(metanav, #395).Verification
Everything above was checked against the templates, the routing modules and a local estate (SASjs Server desktop + JS mocks + the built app behind a same-origin proxy), including a run that showed
#/review/approverendering withThere are no approvals remaining. That estate would not render the nav tree or the viewer's toolbar for me, so it could not take the spec through an actual file download - the end-to-end proof is this PR's CI run, where the six viewer cases exercise real files and test 1 exercises the audit zip.If CI names a further stale step, the log will say which; I have only claimed what I could check.
Closes #398
Both selectors in download.cy.ts are stale or ambiguous, which is why the spec fails as soon as it runs: - `.btn.btn-success` also matches the editor toolbar's buttons (`btn btn-sm btn-success`), and those sit inside a fixed-position container on the approve screen. The first match was one of those, so `should('be.visible')` retried against an element that is never visible and the error read like the download button was covered. The approve row's button is `btn btn-success` with aria-label="Download audit file", so select it by that label. - `.btn.btn-sm.btn-outline.filterSide.dropdown-toggle` cannot match anything: the viewer's options dropdown is `class="btn btn-sm btn-outline filterSide"`, with no dropdown-toggle class. filter-panel.cy.ts and row-cell-limits.cy.ts already select it as `.filterSide`. The spec goes back into the CI list, appended at the end because it needs approvals to exist and the earlier specs create them. metanav stays excluded (#395). Closes #398Three separate reasons the spec could not pass, all of them stale routes or selectors rather than an overlay covering the button: - test 1 opened #/approve/toapprove, which is not a route. The review module is namespaced under review/, so the approve screen is #/review/approve. The dead URL rendered the 404 page, and the only .btn.btn-success on that page is not a download button - which is why the failure looked like a covered element. - on the right route the approve grid still renders nothing on a fresh estate: it draws a download button per table waiting for approval, and there are none ("There are no approvals remaining"). Test 1 now submits an excel fixture for mpe_x_test first, the way stage.cy.ts does, so it owns the approval it downloads instead of depending on what earlier specs happen to leave behind. - the viewer's options menu is a list of div[clrDropdownItem] entries, not buttons, so cy.get('clr-dropdown-menu button') never matched; Download is now selected by its text, as filter-panel.cy.ts selects Filter. The dropdown-toggle class the helper also required is gone from that element too. The download button is selected by aria-label, which is unique, rather than a bare .btn.btn-success that also matches the editor toolbar's small buttons. Closes #398The screenshot - this is the approve screen on a freshly seeded estate, which is what the spec was looking at:
"There are no approvals remaining" is the whole of test 1's original failure: the grid draws a download button per table waiting for approval, and a seeded estate has none, so
[aria-label="Download audit file"]could not exist no matter which selector was used. The test now uploads a fixture tompe_x_testand submits it first, then downloads the audit file for the table it just submitted - targeting the button whose id matches the id in the stage URL, and asserting exactly one.I could not get a viewer screenshot out of this estate: the app gates on the evaluation terms, and every
#/view/...visit here lands on/#/deploywith the terms unaccepted, so there is no.filterSidetoolbar to open. I am not going to describe a modal I could not photograph - the option labels are quoted from the template instead:<option value="EXCEL">Excel (.xlsx)</option>, which is whycy.select('Excel')never matched.Where the pipeline stands. Run 1: 7 failing. Run 2 (
847d329): 5 failing, and the failures were one more stale assumption -cy.select()matches the whole option label, soExcel,SAS Datalines,SAS DDL,TSQL DDLandPGSQL DDLmatched nothing.3e4070aselects by value (EXCEL,SAS,SAS_DDL,TSQL_DDL,PGSQL_DDL), which is set in the template and does not move when a label gains a qualifier. That run also confirmed the two cases that were already converted: audit-file download and viewer CSV both passed.Your three points, all in
3e4070a:submitExcelno longer takes a callback nothing passed;attachExcelFilestarts fromlabel[for="file-upload"]rather than.buttonBar button:last-child.have.length(1)rather than indexing[0]into every match on the screen.cypress-specs.shno longer says the spec depends on approvals left by earlier specs - it seeds its own - and now says the ordering is only "a spec added later goes on the end".Both fixed in
628092d.fixturePathis'excels'now, so the template supplies the single separator andattachFilegets/excels/regular_excel.xlsx.That resets the SAS/cypress job on this PR - the previous run was 5 failing at
cy.select(), which3e4070aaddresses by selecting the format by value. Nothing else is outstanding from review on this branch.Lighthouse failed on
628092dwithNO_NAVSTART- "Something went wrong with recording the trace over your page load. Please run Lighthouse again." That is the trace recorder, not a score: every audit that depends on the trace bailed out and the job exited 1. This commit touches only a Cypress spec, so it cannot reach the built app Lighthouse audits.I reran that run rather than touching the commit. Ng-test is green on this head and the SAS/cypress job is still going.
Third run: 3 passing (csv, excel, SAS Datalines), 4 failing, and both remaining causes are now clear.
test 1 timed out on
label[for="file-upload"]- my change from the review. The label is only a click target for a hidden input, andattachFilecan drive the input itself, so the helper no longer clicks anything: it selectsinput#file-upload, asserts there is exactly one, and attaches. Stable attribute, no positional pick, no dependence on a control that is not always rendered.tests 5-7 (the three DDL flavours) timed out waiting for the file, and this one is not the spec.
services/public/getddl.sasnames the download<libref>_<ds>.ddl, which is what the viewer expects and what the spec computes from the button id (tableTitleisLIB.DS, soreplace('.', '_')). The mock names it after the libref alone -DC_JSLIB.ddl- so the file the estate wrote never matched the file the test waited for.sas/mocks/sasjs/services/public/getddl.jsnow follows the service.Both in
f50a0de. Worth noting the mock divergence is the kind of thing that only shows up when a spec is allowed to run, which is this PR's whole point.All three in
468d2e9.attachExcelFilenow asks the body which modal is there: no.clr-abort-modalmeans an ordinary upload, so it confirms with the footer button; an abort modal means it asserts the title is not "Abort Message" and stops. No waiting on a modal that may never appear, and no clicking through an overlay.div[clrDropdownItem], so the item is selected by that attribute plus its text -cy.contains('clr-dropdown-menu [clrDropdownItem]', 'Download')- rather than the deepest node holding the words.mpe_x_testand the approval it creates stays. The comment now says the spec seeds its own approval and leaves it behind, and keeps the ordering reason separate from that.Two more screens, both from the app itself, to show what the failing cases were looking at.
The route the spec used to open -
#/approve/toapprove, which is not a route:The review shell renders, the sub-nav renders, and the content area is the router's 404. That is why test 1's old
.btn.btn-successassertion matched something that was never a download button, and why the error looked like a covered element.The live route -
#/review/approve, with no approvals on a seeded estate, is the image already in this thread: "There are no approvals remaining". Both together are the whole of test 1's original failure.On the download modal itself. I could not photograph it, and the reason is visible rather than a matter of effort:
The table opens -
DC_JSLIB.MPE_X_TEST (7 rows, 9 cols)is in the toolbar - but the grid is replaced by "The license key for Handsontable is missing", so the viewer's toolbar and its options menu never render on this estate. That is a local-only gap: the CI estate runs with a licence, which is why the same spec reached the menu there and failed later, atcy.select()and then at the file name.No video either, for the same reason: every recording I could make here stops at the licence notice. The CI videos exist but this Gitea does not expose run artifacts -
/dc/dc/actions/runs/{id}/artifacts/{name}answers 404 for both a token and basic auth - so I cannot pullcypress-videos.zipfrom the run that has the real failure in it.No size limit was in the way - Gitea here allows attachments up to 50 MB and
.mp4is on the allowed list, and the CI artifact fetch was an authorisation 404 rather than a size rejection. I had attached a short recording of the dead route and the live route; I have taken it down, because video belongs onbin.4gl.io/paste.4gl.io(which expire) rather than in this instance's storage.The screenshots above stay: they are a few tens of KB each and they show the two states test 1 was written against - one of which does not exist.
On the licence. The free-tier key is in the code, not missing:
licence.service.tsdefinesDEFAULT_HOT_LICENCE_KEY = 'non-commercial-and-evaluation'and falls back to it whenever a licence carries nohot_license_key. My estate'slicence.jsonis empty, so the default should apply - and it does not reach the grid: Handsontable renders "The license key for Handsontable is missing" as an overlay over the table area, and that overlay is what swallows the click on the options menu item. The menu itself opens fine (four items: Viewboxes, Filter, Download, Web Query URL - confirmed by counting[clrDropdownItem]in the DOM), so the modal below it never gets the click.That is why there is no modal screenshot: not a size limit and not a missing key, but the free-tier default not reaching the grid on this estate. On CI the same menu and modal do open - tests 2 to 4 got as far as
cy.select()and then the file name - so this looks local, but it is worth a look on a free-tier estate, because the notice is an overlay that eats clicks.In
1c0d636. The abort branch no longer asserts that the title does not contain "Abort Message" - which, as you say, only fails on the wording. It throws with the title instead:So the report names the upload, whatever the modal happens to say, instead of the run continuing into a timeout that blames the next command.
All three in
b47fb23.1. The one-shot body read. Right - a slow upload made
abortModalfalse and skipped the branch entirely. It now waits for whichever appears, withlongerCommandTimeout, then branches on what matched:and the confirm click carries the same timeout instead of the default.
2.
dsin the mock. Guarded withtypeof ds !== 'undefined', so a call without adsin scope falls back to the libref name instead of throwing a ReferenceError.3. The leftover approval. I checked the workflow before choosing: the CI job deletes the appLoc folder and re-runs
makedatabefore the specs run (build.yaml, "Deploy mocked services"), so the pending row does not accumulate between runs - it is one row per run, and a hand-run against a reused estate leaves one per run. Both comments now say that, and why it does not affect the assertion: the button is anchored on the id this test created, so a stale row can neither make it pass nor make it fail. Cleaning it up would mean driving Reject through the UI after the download, which adds a step whose failure would mask the assertion it follows - the policy is recorded instead, as you offered.No finding taken on the
.select()keys or the route change, noted.All three in
0ae552e.1. Trailing slash on the id. The stage URL slice now trims it:
url.split('/stage/')[1].split(/[?#]/)[0].replace(/\/+$/, ''), with a comment saying why - a route rendered as/stage/<id>/otherwise produces an id that cannot match the button'sidattribute, and the failure reads as a selector mismatch rather than as the route.2. Ambiguous menu label. Matched on the whole label now,
/^\s*Download\s*$/, so an entry like "Download web query" cannot makecy.contains` ambiguous - it fails outright in that case rather than picking one, which is the behaviour you flagged.3. The guard was lopsided. Both hosts are behind a
typeofcheck now:So a call with either variable out of scope falls back to the plain name instead of throwing the ReferenceError the comment was there to prevent.
Merged in
4c61ef3- the click-handler note and the anchored-label note describe the same selector, so they are one block now. That was the only item, so the branch is otherwise as reviewed.Separately,
Build-and-ng-testfailed on the other PR (dc/dc#412, SAS only) at theCheck auditstep with1 high severity vulnerabilityin the client'sfollow-redirects. This branch's identical job passed at 4m51s minutes earlier, so it looks like registry/advisory drift rather than anything in either diff; I am rerunning it there rather than folding a dependency bump into a SAS-only PR.CI is red at
Check audit, not in this diff:follow-redirects 1.16.0-1.16.1(GHSA-mrwp-f3q8-3g9p, high) arrived upstream today and the gate fails on any runtime advisory. The fix is a lockfile-only bump in dc/dc#414 (1.16.1 -> 1.16.2). Once that lands I will rerun this PR's jobs rather than rebase, so the review history stays intact.The development job came back failed, and the log names the cause - it is my spec, not the infrastructure.
Cases 2-7 - every viewer download, CSV through PGSQL DDL - pass, so the route, the select-by-value fix and the download assertions are sound. The one failure is the audit-file case, and the cause is that the editor renders
#file-uploadinside its upload modal:editor.component.html:298-306puts the Upload button ((click)="onShowUploadModal()") in the button bar, and the input at:78is only in the DOM once that modal is open. I had removed the click that opens it.189e304(rebased onto the merge as39b12e9) clicks the button first, by label rather than by position -cy.get('.buttonBar button').contains('Upload').click()- which is whatexcel.cy.tsdoes and why its 24 cases pass.I did not verify this locally: the mock estate has no upload service, so a local run would fail at the same place for an unrelated reason and tell me nothing about the selector. The CI run is the honest test, and it takes ~30 minutes.
Progress from run 4025, and all three of your points are in
5eb8b0b.The upload now parses - the Upload click fixed the missing input - and the next failure is mine:
The info modal and the abort modal are the same component (
info-modal.component.html, classclr-abort-modal), so my branch treated a successful parse as an abort. The title is what distinguishes them, so the helper now asserts it -should('include', 'Table Found')- and closes the modal. Anything else (an abort, a validation error) fails the case with its own text rather than being clicked through.Your first point, the submit race. You are right that
cy.url().should('include', '/stage/')was already satisfied before the submit landed.submitExcel()now clicks Submit and then waits for.preview-submitto disappear - the editor leaving preview mode is the signal that the submit was accepted - before the URL is read for the id.Your second point, the comment. Trimmed: it now states where the approve screen is and why a leftover row is harmless, without the route history.
Still open, and worth stating plainly: this is the third attempt at this one case, and each failure has been a different layer (missing input, then modal classification). If this run fails again I will stop guessing from the log and drive the estate by hand to watch the flow, rather than push another speculative selector fix.
REVIEWED ON 2026-10-31 12:23 UTC
Reviewed the full base...head diff: enabling the download spec in .gitea/scripts/cypress-specs.sh, the rewritten selectors and upload/submit helpers in client/cypress/e2e/download.cy.ts, and the filename change in sas/mocks/sasjs/services/public/getddl.js. The new option values (EXCEL, SAS, SAS_DDL, TSQL_DDL, PGSQL_DDL) match viewer.component.html:183-189; [aria-label="Download audit file"] and [id] match approve.component.html:135-137; 'Table Found' matches editor.component.ts:816; libref_ds.ddl matches the production outname at getddl.sas:54.
client/cypress/e2e/download.cy.ts:53 - the new comment says a leftover approval "neither helps nor hinders" the case, but the approve datagrid paginates at 10 (approve.component.html:145, itemsNum in approve.component.ts), so once a hand-reused estate carries more than a page of pending approvals the row this test submits is not rendered and
should('have.length', 1)fails. Either raise the page size for the assertion / clear pending approvals first, or narrow the comment so it does not claim more than the grid allows.sas/mocks/sasjs/services/public/getddl.js:8 - the added
else ifbranch returns a library-level DDL whendsis absent, while production getddl.sas aborts in that case (getddl.sas:43-45, "Dataset &libref..&ds was not found"). A client regression that droppeddsfrom downloadDDL (viewer.component.ts:532) would therefore stay green against the mocks. Mirror the abort instead of the fallback.CI: Build-and-ng-test is green, Lighthouse and Build-and-test-development are still pending - the job that actually runs this spec has not reported, so the selector work is unproven at review time.
2 findings above for review.
Both findings in
86c8a1a, and CI moved the failure one layer further on - to the approve grid.The failure at
5eb8b0b:so upload, parse, "Table Found" and submit all work now; the approve grid simply did not have the row when it loaded. The screen now reloads up to five times, two seconds apart, until the row with this test's id is present - which also addresses the race you raised earlier from the other direction, by making the assertion wait for the approval rather than for the submit.
1. The leftover-row comment. You are right that it claimed more than the grid allows. It now says the rows accumulate on a hand-reused estate and the grid paginates, so a row pushed past the first page would not be found; the anchored id stops a stale row satisfying the assertion, but cannot rescue a row that is not rendered.
2. The getddl mock. Changed to abort when
dsis absent, matchinggetddl.sas:43-45, rather than returning a library-level DDL. Good catch - that fallback would have hidden exactly the client regression you describe (viewer.component.ts:532droppingds).I am not claiming this run is green; if
86c8a1afails again I will drive the estate by hand rather than reason from another log.That is very likely the whole thing, and it also explains why the retry failed rather than the first attempt alone:
retries: { runMode: 1 }means the second attempt uploads a fixture the first attempt has already loaded, so there are no changes to approve and the case reports a missing approval instead of a duplicate upload.fbf0ed6:cy.task('makeUniqueExcel')readsregular_excel.xlsx, renumbersPRIMARY_KEY_FIELDfrom a random base and writes a copy;afterEachvia a newdeleteFiletask -deleteFoldercannot be used because the source fixture sits in the same directory.Checked the renumbering on its own before pushing: 472 data rows, keys running from the random base, header row untouched.
This cannot be confirmed locally - the client at
:8080is only the Sasjs Deploy screen, with no library tree to upload into - so the CI run on this head is the verification.Found it, and it was not the fixture - it was the licence.
loginAndUpdateValidKey()without the force flag leaves an unlicensed estate alone on purpose, and the free tier disables file upload (free-tier.config.ts). The key, once planted, persists in the estate config, so whether this spec could upload at all depended on whether an earlier spec in the same job had planted one -licensing.cy.tssits in the middle of the spec list and would have. That is why CI's failure was at the approval rather than at the file input, and why my local run against the mock estate never got past the modal.1455f9dcallscy.loginAndUpdateValidKey(true), so the spec plants the key it needs:Order is safe:
csv-limited(which asserts the free-tier modal) runs first incypress-specs.sh,downloadruns last, so nothing downstream inherits the key.Both fixes stay: the renumbered fixture removes the retry no-op, and the licence removes the dependency on spec order.
The failure names its own cause, and it is not the fixture or the licence:
DC20261031-201657.470_601010_4449is a load reference, not a table id. The case took the id from the stage route (/stage/<id>) and looked for[aria-label="Download audit file"][id="<that>"]- but an upload lands on the stage route by load reference (viewer.component.ts:1248usesrow.LOAD_REF; only the editor's editor-path uses the table id). No number of reloads could produce a button with that id, which is why the row "never appeared" while the approval was sitting there.dff4bbdfinds the row the wayhook-programsdoes it - by the table it names - and reads the id the download is named after from the button itself. I also left the renumbered fixture and the licence in place: the first removes a retry no-op, the second removes the dependency on spec order, and this third one removes the wrong identifier.Verified locally against the mock estate (licence planted by the spec):