Review measured this and the measurement settles it: with the vendored Clarity
CSS the label's content box already sets its container's height, so the rule's
`align-self: start` has no cross-axis space to move into and `height:
fit-content` resolves to the same 24px. The base layout also has a 28.5px gap
between the label's bottom and the first row's top, so there is no shared strip
for the rule to hand to either element - it changes nothing while claiming to
decide which of two overlapping targets a click reaches.
A rule whose comment describes a behaviour that cannot happen is worse than no
rule, so it goes. My own replacement attempt and the case 13 assertion that went
with it are in the same position - the assertion passed at the base commit, so it
could not have caught any of this - and neither is here.
What remains is the honest state: the overlap Cypress reported is not explained
by these boxes, and the next attempt needs rects from a rendered page rather than
another plausible rule. #521 tracks that.
Two of the three findings open on the release pull request.
`runs()` piped `--version` through `head -1` with the streams merged. A browser
that works still warns on stderr - GPU, dbus, the root-without-sandbox notice -
so the first warning was read as the version, the `chrom` match failed, and the
script installed libraries it did not need before exiting 1 with nothing
exported. Under `set -o pipefail`, closing the pipe with `head -1` did the same
to any browser printing more than one line.
The version now comes from stdout and the warnings are kept apart for the
failure message, which is where they are worth having. Both cases are pinned by
new tests: a browser that warns on stderr and prints two stdout lines passes, and
the browser that cannot start still reports its loader error.
`dc-lint719` was a gitlink at the repository root with no .gitmodules entry, so a
clone produced an empty directory and `git submodule update --init` reported "No
url found for submodule path". Removed from the tree.
The production budget allowed 13mb warning and 15mb error for the initial
bundle, while the split brings it to about 4.3mb and the pre-split build was
about 9.7mb. So restoring any one route to a static import would have re-inlined
its module and the build would still have passed - the split would drift back
with nothing to notice it.
4.6mb warning and 5mb error sit above the split with headroom and below a single
collapsed route, which is the point of a budget.
The rule moves the intersection from the library label to the child row rather
than removing it, so the comment has to say what a pointer gets there: the row.
Collapsing the label's box so the two boxes stop sharing pixels is the fix that
needs the rendered geometry.
The routes declared `loadChildren`, but with a reference to a module imported at
the top of the routing file - so every routed module, and everything it pulls
in, was in the initial bundle. `import()` in the route declaration is what makes
the router fetch a chunk instead.
Weight, from the production build:
main 8.41 MB -> 2.95 MB raw (2.03 MB -> 588 kB transfer)
viewer chunk 1.06 MB + 809 kB + 720 kB + 123 kB
editor chunk 1.46 MB + 687 kB + 512 kB
home chunk 1.98 MB, and the routes under it their own
Measured on the compressed 4.14.0 estate before the split came out and after it
went in: `#/home/tables` 0.52 -> 0.93, `#/view/data` 0.53 -> 0.80, the editor
0.31 -> 0.52.
The reason the earlier attempt was reverted was the suite, not the split: the
tree walk clicked a node captured a moment earlier, and fetching the route's
chunk re-renders the tree in between. That walk is now `cy.openTableFromTree`,
which re-queries at each step, and this lands on top of it.
The library label and its child table rows are projected into one content
container, and the label can paint over the first child row - which is why
Cypress refused the viewbox walk with "this element is being covered by another
element", the covering element being the library's own interactive label.
Nested content now sits in its own stacking context, and viewbox's click is no
longer forced: the specification states the click path a user takes, or it fails.
Local verification was inconclusive - the renderer is unstable on this machine
under the load it is carrying - so CI is the evidence for this one.
Review 900 on #520, three findings.
1. getcolvals used the stripped name for the shape test and then dropped it:
namedLibref/tableName were split from the unstripped libds, so
DC_JSLIB.MPE_TABLES-FC passed validation and was refused as a missing table.
The stripped pair now drives both, and the -FC case gets the answer the
estate gives. Recorded first, because it differs from getrawdata's:
getrawdata DC634301.MPE_TABLES-FC -> syscc=1012 MP_ABORT
getcolvals DC634301.MPE_TABLES-FC -> syscc=1012 - on macro entry MP_ABORT
A registered NAME-FC is a catalog (getcolvals.sas reads it through
%mp_cntlout into work.fmtextract), so the mock reads the catalog member;
anything else is not a catalog and gets the recorded abort. New gated
scenario public/getcolvals/catalog_suffix_on_dataset.
2. the LIBDS rule was the third copy of one expression. It now lives in
services/dcLibds.js - libdsWithoutCatalog() and isValidLibds() - and getcols,
getcolvals and getrawdata all eval it, the way the estate keeps it in
%mp_validatecol. getrawdata could not share it through dcMockUtils (it does
not load that file, by design), which is why the rule has its own module
rather than a fifth function in the utilities. A static test fails if a
service carries its own copy again, and asserts which three services load it.
3. a table file that parsed but carried no columns reached
`tableData.columns.filter(...)` and died with a TypeError. loadTableFile now
rejects a document without columns and rows arrays, naming the file, beside
the existing invalid-JSON error.
Tests: 152 pass. Both behaviour changes were run against the previous code
first and fail there (unstripped names in getcolvals, no table-shape check), and
parity/check.js is ok on all 26 scenarios.
Review found two things in the aspect-ratio change and the new command.
The `.without-text` rule fits `images/dc-logo.svg`, which declares 70x70 - a
square - not the wordmark's 1690x240. An explicit aspect-ratio overrides the
intrinsic one, so that box reserved 30x4.3px and the square logo was then drawn
into it. It is `1 / 1` now, and the comments name the file each rule sizes.
`cy.openTableFromTree` looked up the table with an unscoped
`cy.contains('.clr-treenode-link', tablename)`, which searches the whole
document: the same class is on column nodes, and two libraries can hold a table
with the same name, so `libNameIncludes` decided only which library got
expanded. The lookup runs inside the re-queried library node now.
The command no longer forces its click; viewbox keeps its own forced click,
because the element covering that tree's table button is the parent library's
interactive label and that overlap is a real defect to fix rather than to click
around.
Thirteen specs carried the same local helper: find the library node whose text
contains a fragment, expand it, then click the table whose name contains
another, by iterating a jQuery collection captured a moment earlier and calling
.click() on the node that matches.
That captured reference is the reason the suite is fragile under a route that
fetches its chunk: expanding a library replaces its subtree once the child
tables load, and a route that is still fetching re-renders the tree again, so
the node clicked is a node the DOM no longer holds.
The command re-queries at each step - filter the tree nodes by text, then
filter the table links the same way - and ends by waiting for the header's
per-column dropdown, which is what proves Handsontable has populated the header
rather than just the row shells. Specs call cy.openTableFromTree(library,
table); the two specs that need a callback of their own (licensing, liveness)
and viewbox's variant keep their local helpers for now.
Not verified locally: the Cypress renderer crashed under the load this machine
is carrying, so CI is the first run of this.
Reviews 893 and 894 on #518.
894-1 and 894-2 - a -FC request named a format catalog, and the mock answered
it with the sibling dataset. Stripping the suffix for validation made the name
canonical but also changed which object was resolved, so LIBREF.NAME-FC returned
NAME's rows for a name that is not a catalog at all. Now the MPE_TABLES
registration decides: if it carries a row for NAME-FC the member is a catalog
and is streamed as LIBREF.NAME.ext; if it does not, the request is not a catalog
and the estate's own answer is reproduced - syscc=1012 from %mp_cntlout,
recorded as public/getrawdata/catalog_suffix_on_dataset. The catalog the fixture
does hold, MPE_X_CATALOG, has no format entries, so its listing is empty; that
is recorded in the parity README beside the other fixture divergences rather
than left as an unexplained empty body. The test that pinned the object swap now
asserts both halves.
894-3 - getcolvals answered a well-formed libref it does not have with "Invalid
libds", and a malformed one with "Target table not found": the two rules were
the wrong way round. getcolvals.sas validates the SHAPE first with
%mpe_validatecol(...,LIBDS,...) and that is the only "Invalid libds"; the table
is then left to %mp_derivedvars, which aborts "Target table not found: <LIBDS>"
with its own MAC. Both are now recorded: public/getcolvals/wrong_libref uses a
valid-length NOLIB and pins the resolution abort, and the new
public/getcolvals/malformed_libds pins the shape abort with an 11-character
libref. fault-paths.test.js went with them.
893-2 - loadTableFrom duplicated loadTableData, so the two could drift: one
loadTableFile(libref, tableName) now serves both, with the control library as
the default.
893-3 - an unreadable or malformed table file was swallowed into null and
surfaced as "Target table not found". It now throws naming the file, because a
broken fixture should not send the next reader looking for a missing table; an
absent member still returns null and keeps the recorded abort.
Tests: 149 pass. Each of the three behaviour changes was run against the
previous code first and fails there - swallowing parse errors, answering the
catalog with the dataset, and restoring the inverted getcolvals rule.
parity/check.js ok on all 25 scenarios.
viewbox's tree walk selects `.clr-treenode-link` and clicks it. In the table
tree the node's own label, a `<p class="m-0 cursor-pointer">`, is painted above
that button, so Cypress refuses the click with "this element is being covered by
another element" and the specification that walks from the base table to a
viewbox fails on it.
The button is what carries the click handler, so the click is forced rather than
aimed at the label on top. The overlap itself is worth a separate look: if the
label covers the button for a real pointer too, the node is not clickable in its
upper area.
Lighthouse on the built app reported cumulative layout shift 0.447, and the
unsized-images audit named both logos: images/datacontroller.svg in the header
(on every route) and the same file in the login sidebar. The SVG declares
1690x240, but an <img> with only a CSS width reserves no height until the file
arrives - about 30px of header reflow when it does.
aspect-ratio on the three rules that size those images makes the browser reserve
the box up front, so nothing moves when the file lands.
Measured on the built app served by the 4.14.0 estate, desktop preset, one run
per URL: CLS 0.447 -> 0.011 and 0.004 -> 0.015. Performance scores were not
stable across runs on this machine today, so this claims the layout shift, not a
score.
Eight `cy.visit` calls in five specs addressed the app as `${hostUrl}/...` or
`cy.visit(hostUrl)`. That is the address the app had on a dev server, which
served it at the origin root; served by the server it talks to, the app lives
under `appLocation` and the root is not the app, so those visits loaded nothing
the spec could act on and then failed waiting for a grid, a modal or a rendered
state.
Every visit now builds its URL from `hostUrl + appLocation`. The three specs
observed failing in CI pass with it, against the build streamed at
/AppStream/clickme:
browser-info.cy.ts 3 passing (#hotTable never found)
startupservice-launcher.cy.ts 1 passing (.clr-abort-modal never found)
deploy-folder-picker.cy.ts 2 passing (no state to act on)
The specs opened with `cy.visit(`${hostUrl}/SASLogon/logout`)`. Against a dev
server that only navigated the single-page app: every path answered index.html,
so nothing was logged out. With the app served by the server it talks to, the
route answers 403 - `desktopRestrict` in @sasjs/server refuses it by design, and
desktop is that server's default mode - so the visit failed the before hook and
every spec aborted on its first line.
`cy.clearSession()` clears cookies and localStorage, which is what the visit was
standing in for, and needs nothing from the server. The MODE=desktop line and its
comment go with it: the server already defaults to desktop, so the line changed
nothing and the comment described a mode the job never ran in.
Verified locally against a 4.14.0 estate with the build streamed to
/AppStream/clickme and the mock services seeded: viewer-labels 5/5 (the spec that
failed in CI), filtering 10/10.
Also in this push, three review findings on the harness itself:
- resolve-browser.sh searches ${DC_PATH:-$PATH} through its own loop, so the
test pins the whole decision to fixtures instead of inheriting a chromium from
the host; case 2 was passing here and failing on any machine with one.
- the Cypress step requires the exported path (`${CYPRESS_BROWSER:?...}`) rather
than falling back to a browser name, so a missing export fails with a message
naming the step that should have set it.
- the step asserts the app is served at /AppStream/clickme before the suite
starts, because a deploy that streamed no frontend would otherwise surface as
twenty-six navigation failures.
Two runs an hour apart picked different images: b5bd8716 landed on a runner with
this org's job image and its Karma tests passed with the Chromium that image
carries, while bafa0103 landed on one running the stock public image and died
with 'Can not find the binary /usr/local/bin/chromium'. Runners differ, and will
keep differing as shapes change, so neither a fixed path nor a fixed browser
name is safe - hard-coding either turns an unfamiliar runner into a red
pipeline.
.gitea/scripts/resolve-browser.sh detects rather than declares:
- a browser on the runner -> use that path, log which one, its version and
that it came from the runner
- nothing it recognises -> fetch the pinned Chromium snapshot, verify its
sha256, and log that it did
Detection looks at chromium, chromium-browser, google-chrome and
google-chrome-stable on PATH and in the usual absolute locations, so a runner
that ships a browser under any of those names needs no configuration change.
Cypress, Karma and chrome-launcher all accept a path, so the resolved path is
what gets exported: CHROME_BIN, CHROME_PATH and CYPRESS_BROWSER. A line also
goes to the job summary, so the choice is visible in the run rather than buried
in a log.
The browser is then proved to start before it is handed over. That last part is
from the first run of this change: the fetch worked, the symlink worked, and
Karma still failed with 'error while loading shared libraries: libnspr4.so' -
a snapshot is a bare binary tree, and the libraries a Chrome .deb would have
pulled in are not there. So the shared-library set is installed as part of the
fetch, and a browser that still will not report a version is refused with its
own error message instead of being exported.
The pin is the artefact runner.4gl.io's image installs: snapshots revision
1717300, Chromium 157.0.8103.0, sha256 542445ec... Verified by fetching it here
- 240MB, checksum OK, chrome-linux/chrome inside.
Its test pins four branches - found, found under another name, found but not
runnable, and nothing found - and runs in the build job beside the changed-path
matcher: the wrong answer here reads as a broken image rather than a broken
decision, which is exactly how it cost an hour today.
Serving the app from the server instead of a dev server exposed something the
dev server had been hiding: the app is configured loginMechanism="Redirected",
and an unauthenticated visit navigates the browser to /SASLogon/logout, which
answers 403 on a server-mode estate. ng serve absorbed that navigation by
serving index.html for every path, so the suite never saw it.
In desktop mode there are no accounts and no permission model - one fixed user -
so /SASjsApi/session answers without auth and there is nothing to redirect for.
Verified locally on 4.14.0 before pushing: streamed build at
/AppStream/clickme, MODE=desktop, seeded with makedata + createdemodata, and
liveness.cy.ts passes both tests ('Login and submit test' 39.6s cold,
'Recovers from a failed startupservice request' immediately). Without the seed
the nav tree is empty and the same spec fails, which is worth knowing.
The fleet now runs its jobs on the pinned image from runner.4gl.io, which
carries Chromium at /opt/chromium/chrome-linux/chrome with chromium and
chromium-browser symlinked into /usr/local/bin. Three jobs were still
downloading google-chrome-stable_current from Google on every run:
- Build-and-ng-test: step removed, CHROME_BIN points at the image's chromium
(Karma finds no browser by name otherwise)
- Build-and-test-development: the .deb download goes, the apt library install
stays as the safety net; --browser chrome becomes --browser chromium
- lighthouse: step removed, CHROME_PATH set, because lhci launches the browser
itself through chrome-launcher and would not pick up a binary called
chromium on its own
The browser is now the version the image pins and verifies by checksum rather
than whatever Google serves as _current on the day, and three downloads per
run are gone.
The job already downloads the client-dist artefact and streams it to the
server at /AppStream/clickme, then starts an ng serve it could have used
instead. The suite now visits the streamed app:
- hosturl/appLocation are passed to cypress as --env values pointing at
http://localhost:5000/AppStream/clickme
- the three index.html patches that pointed the dev build at :5000 are gone;
page and services share one origin, so the repository's serverUrl=""
(relative requests) is what ships
- ng serve and wait-on are gone with them
- CORS and its WHITELIST go too: with no second origin left, the server no
longer needs to allow one
The old station rewrote cypress.config.ts with --regex='"hosturl".*', which
never matched the single-quoted hosturl line and was a silent no-op while the
default was the value it wanted; removing ng serve left the suite visiting a
port nothing listens on. --env cannot silently miss.
This is the artefact Lighthouse audits, so both jobs now measure the same
build, and 26 specs no longer pay a dev compile per navigation.
Reviews 890 and 892 on #517, both about getrawdata.js in this branch.
890 - the -FC strip fed only the shape guard. `withoutCatalog` was computed for
the LIBDS test and then dropped, so the filename, the table name and the library
were all taken from the unstripped value: a request the guard accepted resolved
nothing and came back as an empty download named LIBREF.DS-FC.ext.
getrawdata.sas strips the suffix and then works with the stripped pair
(`table=cats("&lib..",ds)`), so the mock does that too - `requestTable` is the
stripped, upcased name and everything after the guard uses it: the guard, the
filename, the library it reads and the name in an abort.
892 - `Number()` is not ISINT's rule. I checked the premise against the estate
rather than the macro reading, because the two disagree here: it answers
"Invalid filter" for 1,000 and 1abc (which a leading-prefix read would accept),
and for 0x10, which Number() reads as 16. What the estate does accept is
7.0, 1e3, +5, 007, " 7 " and -3, each of which then fails downstream on the
filter RK rather than on validation. The mock now matches the value with a
number pattern first and then asks whether it is whole, which reproduces all of
that; the test's claim is corrected to the recorded answers, and the comment no
longer says the reading was taken from the macro.
Tests: download-headers.test.js is 14 tests. Both new assertions go red against
the previous code - leaving the unstripped name downstream fails the catalog
test, and dropping the shape match fails the filter test on 0x10. Whole mock
suite: 147 tests, 147 pass; parity/check.js unchanged.
Stacks on #517, which fixed the download headers of this service.
The mock loaded every table from the control library's folder, whatever LIBDS the
request named, and answered an unresolvable table with a 200 whose body was the
headers and nothing else. getrawdata.sas assigns the library it was given
(%dc_assignlib(READ,&lib)) and reads the data from there; the control library
supplies only the registration row that names the validity columns to drop. So:
- the table now loads from data/<LIBREF>/, which is where makedata and
createdemodata write each library's files, and the LIBDS is upcased first as
`call symputx('table',upcase(_table))` does
- a table that cannot be resolved aborts before anything is written, with the
message and MAC the estate answers - recorded, not inferred:
NOLIB.MPE_TABLES -> Target table not found: NOLIB.MPE_TABLES MP_ABORT
DC634301.NO_SUCH_TABLE -> Target table not found: DC634301.NO_SUCH_TABLE
WORK.NOPE -> Target table not found: WORK.NOPE
The abort is mp_derivedvars', so the MAC is that macro's own default, and the
message carries the upcased LIBDS rather than the service name.
Recorded: new parity scenario public/getrawdata/missing_table, golden taken from
the synthetic estate (one accepted difference, SYSCC, as for the getddl abort).
Tests: download-headers.test.js gains two - a download of TESTDATA.DEMO_ORDERS,
which lives outside the control library and used to answer an empty file, and
the three unresolvable-table cases above. Both go red against the old code:
loading from the control folder fails the first, and removing the abort fails
the second. Whole mock suite: 146 tests, 146 pass; parity/check.js ok on all 23
scenarios.
Review 886 on #517, both findings.
Finding 1 - getrawdata validates three inputs, and the mock only had the third.
getrawdata.sas aborts, in this order and all with mac=&_program..sas, before it
streams anything:
table shape -> "Invalid table"
filter integer -> "Invalid filter"
type list -> "Invalid type"
The mock now reproduces all three, from the rules of the macros the service
calls: %mpe_validatecol(...,LIBDS,...) strips a trailing -FC and applies
%mp_validatecol's /^[_a-z]\w{0,7}\.[_a-z]\w{0,31}$/i, and
%mp_validatecol(...,ISINT,...) is input(??best32.) with mod 1 = 0, so 7.0 and
1e3 are integers and abc and 1.5 are not. Each answer was recorded from the
synthetic estate first:
{table:'NOT_A_LIBDS'} -> Invalid table
{filter:'abc'} -> Invalid filter
no table at all -> Invalid table (was a
ReferenceError before)
{type:'FOOBAR',table:'NOT_A_LIBDS',filter:'abc'} -> Invalid table
Finding 2 - the tests waited 100ms for a fire-and-forget write. The cause was
the writers, not the tests: getddl, getrawdata and exportdb used the async
fs.writeFile for the headers file, while %mfs_httpheader (fopen/fput/fclose)
and dcMockUtils.mpStreamfile are synchronous. Those three now use
fs.writeFileSync, so the file is complete when the program returns; the waits
are gone, the positive assertions read the file directly, and the absence
assertions mean "never written" rather than "not written yet". No polling
remains in the file.
Also: mock-settings.test.js renamed the control library to DC7QATEST to prove
getrawdata follows the settings file - nine characters, which the estate
rejects as "Invalid table" for a LIBREF. The fixture is now DC7QATST. That was
a test defect the new validation exposed, not a product one.
Tests: download-headers.test.js is 11 tests, and the two new ones were run
against the old code first - dropping either check fails its own test. Whole
mock suite: 144 tests, 144 pass. parity/check.js unchanged.
Finding 1 on #510 was that getddl sent an octet-stream, quoted-filename header
the estate never sends. The same defect was in every other download path, so
this fixes the class rather than the one service. Every value below was read
from the synthetic estate's own response, except where noted.
public/getrawdata, from getrawdata.sas and its recorded answers:
SAS application/text LIBREF.DS.sas
CSV application/csv LIBREF.DS.csv
EXCEL openxml sheet LIBREF.DS.xlsx (from the service source:
PROC EXPORT xlsx fails on the synthetic
estate with syscc=1012, so it cannot be
recorded there)
MARKDOWN text/markdown LIBREF.DS.md
PDF application/pdf LIBREF.DS.pdf
- the filename is the upcased LIBREF.DS plus the type's extension, unquoted
- WEBCSV and WEBTAB send no headers at all: the estate writes those rows
straight to _webout, and a content-disposition would advertise a download it
does not offer
- a type outside that list aborts "Invalid type" with mac=&_program..sas before
anything is streamed, which is what the estate answers for FOOBAR
admin/exportdb: %mp_streamfile(contenttype=TEXT, outname=&dc_libref..ddl), so
application/text and an unquoted <libref>.ddl - the mock sent text/plain.
The shared mpStreamfile helper in dcMockUtils (exportconfig, getauditfile) now
writes one header per line with the filename unquoted, as %mfs_httpheader does,
so the zip downloads stop quoting a name the estate does not quote.
Tests: sas/mocks/test/download-headers.test.js (renamed from
getddl-headers.test.js) now covers all four paths - 9 tests. Each of the new
assertions was run against the old behaviour first and fails there: text/csv
instead of application/csv, a quoted filename, and WEBCSV aborting instead of
streaming. Whole mock suite: 142 tests, 142 pass.
The parity gate masks the whole generated-by comment and carries no response
headers, so neither the local-time fix nor the header shape was visible to any
test. Both are now unit tests in their own file, which fixes TZ to a zone three
hours from UTC so a return to the UTC getters cannot pass by coincidence:
- the printed clock is the UTC clock plus the zone's offset, checked to within
the two minutes a run can take
- the header file holds exactly the two lines %mfs_httpheader writes,
application/text and an unquoted filename
- an aborted request still describes no download
Each was run against the pre-fix behaviour first: the UTC getters fail the
first test and the old single-line quoted header fails the second. Whole mock
suite: 136 tests, 136 pass.
Six findings from the review of #510, each checked against the synthetic
estate's own public/getddl response before being changed:
- the download is an application/text attachment, not octet-stream, and the
filename is unquoted: the estate sends `content-type: application/text` and
`content-disposition: attachment; filename=<libref>_<ds>.ddl`
- the DDL header timestamp is the session's local time, as %sysfunc(datetime(),
datetime19.) is, not UTC - the estate answers 07NOV2025:22:27 where UTC
getters give 21:27, and the projection masks the comment, so nothing else
could see the drift
- the headers are written after the DDL is generated, as %mp_streamfile runs
after %mp_ds2ddl, so nothing is sent before a request that cannot produce a
file has aborted
- the refusal of an unknown flavour was invented: getddl.sas passes it to
mp_ds2ddl, which has branches for SAS, TSQL and PGSQL only and leaves the
file holding its opening comment. The estate answers that as a 200 download,
and the mock now does too; a new parity scenario, unknown_flavour, pins it
- the "all forty control tables" claim in the DDL comment overstated what the
committed goldens show: MPE_TABLES is the table recorded, in three flavours
- getcols: drop the dead `libref = knownLib` assignment
Tests: node --test sas/mocks/test/*.test.js - 133 pass, 0 fail, and
parity/check.js reports 0 differences for every getddl scenario.
The README still listed a LIBDS naming a library the estate does not have as
outstanding work - the mock falls back to searching every libref and answers.
That is done: the library is validated first and the fallback is gone, and the
entry would have contradicted the code whichever of the two pull requests landed
second.
Folds in #515 (the two getcols aborts, the NOLIB scenario and its unit test) so
the mock parity work is one branch rather than one per finding.
getcols.sas answers two different things and the previous commit collapsed them.
"Invalid libds" comes from the shape check (%mp_validatecol - a libref of eight
characters or fewer, a member of 32 or fewer), and only from there. A LIBDS that
passes the shape and cannot be resolved reaches the second abort, msg=syscc=&syscc.
Probed on the synthetic estate, that second abort says syscc=3000 with a SAS
syntax error behind it for a well-formed unknown library, for a missing member in
a known library, and for WORK.NO_SUCH alike - a defect in getcols.sas rather than
a designed message. The mock reproduces it as it is, because that is what the
client renders in production today, and the comment says so rather than
pretending it is designed.
The scenario is re-recorded with NOLIB.MPE_TABLES, which passes the shape check;
the old value, NO_SUCH_LIB, is eleven characters and only ever exercised the
shape abort, so its golden proved less than its name claimed. The unit test now
covers the malformed, the unresolvable and the answerable cases separately.
The index map and the schema are two lists of the same column names, and nothing
else joins them: a later rename in one would leave getddl emitting CREATE UNIQUE
INDEX on a column the table does not have, while the test that exists to check
every table has a key kept passing.
Each uniqueIndex column must now resolve to a column of its own table,
case-insensitively, so a rename on either side fails here instead of in a
download.
Merges release/7.23, which carries the 4.14.0 pinned estate and the lazy-route
revert.
Review findings on this pull request, and one CI failure they predicted.
The CI failure was the case change, not the DDL: getdata's DTVARS, DTTMVARS and
TMVARS still listed col.name, so they read " some_date" while every header and
row key was "SOME_DATE", and the client matches those lists against the headers
case-sensitively - an Excel upload stopped treating those columns as dates
(the Cypress failure was '21958' where '1960-02-12' was expected, across several
specs). They go through payloadKey now, like every other name in the payload.
Also from the review: the payloadKey doc block had displaced mpGetcols' JSDoc,
so mpGetcols was undocumented and payloadKey was described as taking a table's
columns; it sits above the block again. ddlDialectColumn's unreachable null
return is gone, and an unrecognised flavour aborts rather than quietly
answering SAS DDL under a label it was not asked for.
The intermittent suite failure the reviewer saw is the harness pruning a
session directory under a pending async header write; #512 carries the same
fix, and this branch needs it too or the gate fails at random.
getrawdata's CSV header still upcases inline: that service carries its own
helpers rather than loading dcMockUtils, so payloadKey is not in its scope.
getcols.sas validates LIBDS against the libraries it can assign before it reads
anything (%mpe_validatecol, then dc_assignlib), so a LIBDS naming a library the
estate does not have aborts "Invalid libds". The mock searched every libref
instead, so NO_SUCH_LIB.MPE_TABLES answered with the columns of the copy that
does exist in the control library - a request the estate refuses, answered with
data from somewhere else.
The named library is resolved the way the estate assigns it, and a missing or
malformed LIBDS reaches the same abort. A member missing from a library that
does exist is left alone: the estate fails inside its query there, and there is
no message worth copying.
Recorded as public/getcols/wrong_libref.
Also fixes an intermittent suite failure this work exposed: the harness pruned
earlier session directories, so a service whose headers are written with the
async fs.writeFile could land after its directory was removed, and the ENOENT
took the test process down. Each run still gets its own directory - that is what
stops a stale body being served - and the estate is removed when the process
exits.
public/getddl answered the SAS text for both remaining flavours, which the
download menu labels by name. Each is generated from the same table metadata
now, and each was checked against the estate's own output before it went in:
all forty control tables, byte for byte, in all three dialects.
The dialects differ in more than type names. Postgres quotes with double quotes
and declares its key as a second statement after the table; TSQL brackets the
identifiers, carries the key inside the create table as a UNIQUE NONCLUSTERED
index, and turns every column LABEL into a sys.sp_addextendedproperty block
after the GO. Only a format whose name starts with DATETIME becomes a timestamp
in either - a DATE9. or E8601DT26.6 column stays DOUBLE PRECISION or
decimal(18,2), which is what the estate does and is not what a format list would
suggest.
Recorded as public/getddl/mpe_tables_pgsql and public/getddl/mpe_tables_tsql,
so each dialect is a gate rather than something re-checked by hand.
Revert commit 7d5f1e85.
The lazy split makes the Cypress suite fail, reproducibly, on two separate
runs of two different trees that contain it:
run 3441 (cc43512) viewer-labels 1, full-table-search 1, viewbox 1,
row-cell-limits 4
run 3449 (5aa8cd7) viewer-labels 4, full-table-search 1, viewbox 1,
row-cell-limits 4, deploy-folder-picker 7
Both runs show the same failure family, and it is always the click that
selects a node by name:
CypressError: cy.click() failed because the page updated as a result of
this command ... the subject is no longer attached to the DOM
AssertionError: Expected to find element:
#hotTable .ht_clone_top .htCore thead button.changeType, but never found it
AssertionError: ... with the selector: .clr-treenode-link but never did
The tree is re-rendered while the spec is mid-chain: the click starts a
navigation, the route's chunk is fetched, and the node that was clicked is
detached before the chain continues. With the modules in the main bundle there
was no fetch, so the race almost never fired; a chunk fetch widens it enough to
fail four to five specs per run.
Revert restores a green suite on the release branch. The split re-lands with
the specs' click chains broken up so a re-render cannot detach a subject the
chain still holds - that is the change that makes the test robust to the app
doing what it does, rather than keeping the app eager to keep the test passing.
Not reverted: #507 (text-encoding and handsontable polyfill imports), whose own
run 3442 was green with the suite in this state.
Measured cost of the revert: the audit at 3449 read 0.93/0.80/0.79/0.70 on
performance with both the compressed server and the split in place; the split
alone moved those routes by roughly +0.05 when it landed (run 3442 vs 3441),
so the revert gives back that much and keeps the compression gain.
public/getddl answered one fixed Postgres-flavoured body for every request -
DC_JSLIB.MPE_X_TEST whatever dataset was asked for - which is the last read path
the client calls where the mock invented its answer.
It now builds the SAS-flavour DDL from the table file, the way mp_ds2ddl builds
it from dictionary.columns and dictionary.indexes: the header, the create table
with each column's type, format, NOT NULL and label, and the unique index. The
list-output spacing was derived from the estate's own answers and reproduces
them byte for byte for all forty control tables.
Recorded as public/getddl/mpe_tables, so a mock change that alters the text
fails the gate. The header credits a user and a timestamp, so the projection
masks both - a user id is not something to publish, and the comparison is about
the DDL.
TSQL and PGSQL are not generated yet: a caller asking for one gets the SAS text,
and the README says so rather than inventing a second dialect without a
recording to check it against.
A control table on a real estate is created with a unique index - MPE_TABLES on
tx_to/libref/dsn, MPE_X_TEST on primary_key_field, and so on - and mp_ds2ddl
turns that into the CREATE UNIQUE INDEX block of a DDL download. The mock's
tables had columns and rows and no key at all, which is why getddl could only
ever answer a fixed sample.
Each table file now carries `uniqueIndex` (name plus the ordered columns), taken
from the synthetic estate's own DDL: 39 of the 40 control tables have one,
MPE_REQUESTS is the exception the estate also has, and the format catalog marker
has none because it is not a dataset.
The columns also move to the case SAS creates them with. A table declared with
`alert_event` keeps that spelling in dictionary.columns and in the DDL, while
every payload the client reads reports the name upper case. The files now store
the creation case and the services build their keys through payloadKey(), which
fixes the DDL and leaves the payloads alone.
Two metadata gaps went with them: MPE_LOADS.csv_dir and MPE_MAXKEYVALUES.keytable
are NOT NULL on the estate and were not here.
EOF2
echo "--- second commit ---"; git log --oneline -1 | cat
allan
dc