Skip to content

[ZEPPELIN-6649] Add unit tests for the remaining shared utilities (shell + remote) - #5524

Open
huiseong29 wants to merge 1 commit into
apache:masterfrom
huiseong29:ZEPPELIN-6649
Open

huiseong29 wants to merge 1 commit into
apache:masterfrom
huiseong29:ZEPPELIN-6649

Conversation

@huiseong29

Copy link
Copy Markdown
Contributor

What is this PR for?

Adds unit tests for the three utilities left over in ZEPPELIN-6649, following the conventions in zeppelin-web-angular/AGENTS.md.

  • isRecord (src/app/utility/type-utility.ts): the null and array boundaries, primitives, functions, and an object without a prototype.
  • parseTableData (projects/zeppelin-react/src/utils/tableUtils.ts): header/rows split on tabs, header-only input, trailing newline, and an empty cell between tabs.
  • exportFile (projects/zeppelin-react/src/utils/exportFile.ts): extends the existing spec with multi-row CSV assembly, the xlsx branch (file name, MIME type, and header plus rows read back from Sheet1), and the empty-data guard for xlsx and for data without rows.

Spec files only; no production code is changed. The files the issue lists as out of scope (element.ts, css-unit-conversion.ts, line-map.ts) are not touched.

Not pinned on purpose, since the baseline says not to record behaviour that is already questionable: parseTableData trims the whole input, so a leading empty header cell loses its tab ("\tb\n1\t2" gives ["b"]); CRLF input leaves a trailing \r on each cell at the end of a line; an empty string yields one empty column name. I can open a follow-up issue for these if useful.

What type of PR is it?

Improvement

Todos

  • - Add unit tests for isRecord, parseTableData and exportFile

What is the Jira issue?

How should this be tested?

  • cd zeppelin-web-angular && npm run test:shell -- type-utility.spec.ts (10 tests)
  • cd zeppelin-web-angular/projects/zeppelin-react && npm test && npm run lint (18 tests in src/utils)
  • The React suite does not run in pull-request CI yet (see ZEPPELIN-6566), so it was run locally.

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? No.
  • Is there breaking changes for older versions? No.
  • Does this needs documentation? No.

@huiseong29

Copy link
Copy Markdown
Contributor Author

The npm-audit check fails on braces (GHSA-vfj7-8cjw-p6xm), which zeppelin-react pulls in through micromatch -> http-proxy-middleware -> webpack-dev-server / ts-loader.

This PR only adds spec files under src/ and projects/zeppelin-react/src/utils/. It changes neither package.json nor the lockfile, so it does not affect the audited dependency tree.

Running npm audit --audit-level=high locally gives the same result. The flagged packages are all devDependencies, and npm audit --omit=dev --audit-level=high reports 0 vulnerabilities. The unit tests and lint for this change pass locally.

@voidmatcha voidmatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Optional nit: assert the full XLSX media type rather than a substring.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants