add Data Profiler CSV and JSON export - #759
Conversation
cevheri
left a comment
cevheri
left a comment
There was a problem hiding this comment.
Welcome, and thank you for this. Two things in here are the parts people usually miss, so I want to name them before the problems.
You used csvRow and downloadText instead of building a writer of your own. That is the whole point of src/lib/export/csv.ts, and its docstring exists because three exports each rolled their own quoting and got it wrong. And you masked minValue, maxValue and sampleValues through maskValue without being asked. A profiler export that leaked raw values from a column the UI masks would have been a real defect, and you closed it unprompted.
Three things to fix, in this order.
1. bun run format fails, so CI is red before anything else runs. The failing step is Check formatting (Biome). Your file is wrapped at about 80 columns; this repo's Biome config is wider, so the import list and several expressions are split where Biome would keep them on one line. bun run format:fix does all of it. Worth knowing: that step is the FIRST in the job, so everything after it, lint, typecheck, knip, the four drift guards, build, build:lib and attw, has not run. None of it is green yet, it is unmeasured.
2. The two new tests are outside the describe block. Line 816's }); closes it, and your tests come after. So they never get the beforeEach that calls mockGlobalFetch, the profile fetch is never mocked, profile stays null, and the whole {profile && ...} block including the Export button never renders. That is why queryByText("Export") is null.
3. That failure takes 69 unrelated tests with it. Measured on Group 15/16, the 25 files that share one process: 890 pass / 0 fail on main, 821 pass / 71 fail on your branch, and 890 pass / 0 fail again with only your two tests deleted. The collateral lands in admin/OverviewTab (20), AuditTab (15), OperationsTab (11), QueryHistory (8) and four more. Cause: your test throws at its first assertion, so the last line, document.createElement = originalCreateElement, never runs, and every later file in the group keeps your spy. The URL.createObjectURL override is never restored at all. Restore both in a finally, or in an afterEach.
One more, once those pass. QueryHistory.test.tsx:350 is the working version of this exact test and it differs in a way that will bite you next: it drives the menu with userEvent and finds the items with within(document.body), because Radix renders DropdownMenuContent in a portal rather than inside your container.
And a test-quality note rather than a blocker: the CSV case asserts only that createObjectURL and click were called, never what was written. So the headers, the column order, csvRow's escaping and your own masking are all unpinned, and the JSON branch has no test at all. Asserting the CSV text as one string is what would make the masking you added actually hold.
|
Addressed the review feedback: moved the export tests inside the describe block, added userEvent/portal handling, restored global mocks safely, and added CSV content/masking plus JSON export coverage. Updated commits are pushed and ready for CI/re-review. |
|
there are some github actions error, can you fix them |
…here Three jobs were red for two causes, both in the test file. Biome formatting on one wrapped `mockImplementation` call, and `mock.calls[0][0]` typed `[]` because the `URL.createObjectURL` double declared no parameters; the second is what `Engine Smoke - Build Payload` died on too, since `next build` type-checks `tests/` as well. The expected CSV also carried a leading BOM. `downloadText` does write one for `text/csv`, but `blob.text()` is a UTF-8 decode and strips it, which `tests/unit/lib/export/download.test.ts:92` already records. Building the text now lives in `src/lib/export/data-profile.ts`, the shape libredb#740 established for the query-history export, and goes through `jsonText` instead of `JSON.stringify`, so the one JSON serializer stays the one. Its headers, column order, escaping and masking are asserted there as plain strings, and the two component tests keep only the wiring: that each menu item exports in its own format, and that the masking map the screen uses is the one handed to the export.
|
I pushed the rest onto your branch instead of sending you a third round, since what was left was mechanical. The substance here is yours and it stays: the export itself, What I changed, in order.
Worth knowing for next time: I first mocked Verified in a browser against Postgres and MongoDB before pushing. The email column arrives masked in both files and the real addresses appear nowhere in either. Good first contribution, thank you. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@na12334 short note: follow this standard: https://www.conventionalcommits.org/en/v1.0.0/ |
2 participants
Description
Adds CSV and JSON export support to the Data Profiler so users can download computed profiling statistics instead of manually copying the results.
Type of Change
Related Issue
Closes #750
Changes Made
Testing
The changes were made through the GitHub web editor, so the full test suite was not run locally. CI is expected to verify the implementation.
Test Environment
mainScreenshots (if applicable)
N/A
Checklist
bun run test:coverageandbun run coverage:check)src/lib/db/providers/, I updated the matchingdocs/providers/documentation andtests/integration/db/tests in the same PR (provider triad)Additional Notes
The implementation follows the existing export approach used elsewhere in LibreDB Studio. Full test and coverage verification is left to CI because the contribution was completed through the GitHub web editor.