test: cover glossary CSV typed relations across unit, IT, Playwright#27740
test: cover glossary CSV typed relations across unit, IT, Playwright#27740
Conversation
…d Playwright Add round-trip and validation coverage for typed glossary term relations (synonym, broader, narrower, custom types from settings) in CSV import/export. Unit tests in CsvUtilTest exercise addTermRelations serialization edge cases. Integration tests in GlossaryCsvRelationTypesIT add a true export-reimport round-trip, mixed-format per-term API verification, asymmetric relation visibility from both sides, and a custom-relation-type round-trip via GlossaryTermRelationSettings. Playwright specs in GlossaryImportExport.spec.ts drive the UI bulk import/export flow with typed relations and assert the validation surface rejects unknown relation types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds test coverage to ensure glossary CSV import/export round-trips typed glossary term relations (relationType:fqn) across unit, integration, and UI E2E layers, preventing regressions back to bare-FQN behavior.
Changes:
- Added Playwright E2E specs to validate typed relation import (including export verification) and invalid relation type errors.
- Added
CsvUtil.addTermRelations()unit tests covering null/empty inputs, default handling, prefix emission, and sorting/mixed types. - Added integration tests validating mixed-format import, asymmetric inverse export behavior, full export→reimport preservation, and custom relation type round-trip via settings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/GlossaryImportExport.spec.ts | Adds E2E coverage for typed relation import/export and invalid relation type error surfacing |
| openmetadata-service/src/test/java/org/openmetadata/csv/CsvUtilTest.java | Adds unit tests for typed relation CSV formatting behavior in CsvUtil.addTermRelations() |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryCsvRelationTypesIT.java | Adds IT coverage for typed relation import/export behavior and custom relation type round-trip |
| const stream = await download.createReadStream(); | ||
| const chunks: Buffer[] = []; | ||
|
|
||
| for await (const chunk of stream) { | ||
| chunks.push(Buffer.from(chunk)); | ||
| } |
There was a problem hiding this comment.
download.createReadStream() can return null (e.g., depending on browser/context), but the test unconditionally iterates for await (const chunk of stream), which will throw if the stream is null. Handle the null case (as done earlier in this spec) or assert the stream is non-null before reading.
|
|
||
| test('Glossary CSV import preserves typed relations', async ({ page }) => { | ||
| const { apiContext, afterAction } = await getApiContext(page); | ||
| const relGlossary = new Glossary('TypedRelations'); |
There was a problem hiding this comment.
The glossary names TypedRelations / TypedRelationsInvalid are hard-coded. With Playwright configured as fullyParallel: true, this can cause intermittent failures when multiple workers (or reruns) attempt to create the same glossary name concurrently. Prefer including a uuid() suffix/prefix (or omit the name parameter to use the helper’s random default) to keep the test isolated.
| const relGlossary = new Glossary('TypedRelations'); | |
| const relGlossary = new Glossary(`TypedRelations_${uuid()}`); |
| private static final Object SETTINGS_LOCK = new Object(); | ||
|
|
There was a problem hiding this comment.
This file updates the global glossaryTermRelationSettings and only guards it with a class-local SETTINGS_LOCK. Integration tests run with JUnit parallel execution enabled, and other test classes also mutate these settings, so this can still race across classes. Consider using JUnit’s @ResourceLock with a shared lock name for these settings (and applying it in the other settings-mutating tests) to prevent cross-class interference.
Jest test CoverageUI tests summary
|
- Replace naive comma split in findRowByTerm with Apache Commons CSV parser so quoted/escaped fields don't shift column indices. - Replace class-local SETTINGS_LOCK with JUnit @ResourceLock keyed on "glossaryTermRelationSettings" so settings mutations serialise across test classes, and document the contract for future authors. - Add uuid() suffixes to Playwright glossary names to avoid collisions under fullyParallel test execution. - Guard download.createReadStream() against null and use a waiting toContainText assertion on the error cell so a missing selector surfaces a clearer failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🟡 Playwright Results — all passed (17 flaky)✅ 3958 passed · ❌ 0 failed · 🟡 17 flaky · ⏭️ 86 skipped
🟡 17 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
…class flakes GlossaryOntologyExportIT and GlossaryTermRelationIT both flip global RdfConfiguration in @BeforeAll and write synchronously to a Fuseki backend. They were marked @execution(ExecutionMode.SAME_THREAD), which only serialises within the class — other test classes still run concurrently, inherit the enabled RDF state, and contend for the same Fuseki backend, causing the export request to time out at 60s. Add @isolated so JUnit 5 ensures no other class runs while these are running. This matches the pattern RdfResourceIT already uses for the same reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved 3 resolved / 3 findingsExpands glossary CSV test coverage across unit, IT, and Playwright suites. Resolved issues include naive comma parsing, cross-class settings synchronization, and brittle selector usage. ✅ 3 resolved✅ Bug: findRowByTerm uses naive comma split, breaks on quoted CSV fields
✅ Edge Case: SETTINGS_LOCK only guards within this class, not cross-class
✅ Edge Case: Playwright test uses
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
| Map<String, String> typeByTermId = | ||
| imported.getRelatedTerms().stream() | ||
| .collect( | ||
| Collectors.toMap( | ||
| r -> r.getTerm().getId().toString(), TermRelation::getRelationType)); | ||
| assertEquals("synonym", typeByTermId.get(t1.getId().toString()), "t1 should be synonym"); |
There was a problem hiding this comment.
In testImportPreservesMixedRelationsViaApi, Collectors.toMap(..., TermRelation::getRelationType) will throw a NullPointerException if any relationType is null (e.g., if the importer regresses to omitting the default). This makes failures less actionable. Consider building the map imperatively (or mapping through a non-null accessor) so the assertion fails with a clear message instead of an NPE.
| Map<String, String> typeByTermId = | ||
| clone.getRelatedTerms().stream() | ||
| .collect( | ||
| Collectors.toMap( | ||
| r -> r.getTerm().getId().toString(), TermRelation::getRelationType)); |
There was a problem hiding this comment.
Same as above: Collectors.toMap(..., TermRelation::getRelationType) can NPE on null values, which obscures whether the failure was due to a missing default relatedTo type. Prefer collecting in a way that tolerates nulls so the test fails via an explicit assertion with context.
| * (e.g. a deeply nested parent FQN containing a comma) don't shift column indices and break the | ||
| * lookup. Returns the original line text so callers can run substring assertions against it. | ||
| */ | ||
| private String findRowByTerm(String csvContent, String termName) throws Exception { | ||
| String[] lines = csvContent.split("\\R"); | ||
| try (CSVParser parser = | ||
| CSVFormat.DEFAULT.withFirstRecordAsHeader().parse(new StringReader(csvContent))) { | ||
| for (CSVRecord record : parser) { | ||
| if (termName.equals(record.get("name*"))) { | ||
| int lineIndex = Math.toIntExact(record.getRecordNumber()); | ||
| return lineIndex < lines.length ? lines[lineIndex] : null; |
There was a problem hiding this comment.
findRowByTerm uses record.getRecordNumber() as an index into csvContent.split("\\R"). recordNumber is a record counter, not a physical line number, so this breaks if any exported field contains embedded newlines (valid CSV) or if blank lines are present. A more robust approach is to assert on the parsed CSVRecord columns (e.g., record.get("relatedTerms")) instead of trying to recover the original raw line.
| * (e.g. a deeply nested parent FQN containing a comma) don't shift column indices and break the | |
| * lookup. Returns the original line text so callers can run substring assertions against it. | |
| */ | |
| private String findRowByTerm(String csvContent, String termName) throws Exception { | |
| String[] lines = csvContent.split("\\R"); | |
| try (CSVParser parser = | |
| CSVFormat.DEFAULT.withFirstRecordAsHeader().parse(new StringReader(csvContent))) { | |
| for (CSVRecord record : parser) { | |
| if (termName.equals(record.get("name*"))) { | |
| int lineIndex = Math.toIntExact(record.getRecordNumber()); | |
| return lineIndex < lines.length ? lines[lineIndex] : null; | |
| * (e.g. a deeply nested parent FQN containing a comma or embedded newlines) are handled as a | |
| * single logical record. Returns a normalized CSV row reconstructed from the parsed record so | |
| * callers can run substring assertions against it without relying on physical line numbers. | |
| */ | |
| private String findRowByTerm(String csvContent, String termName) throws Exception { | |
| try (CSVParser parser = | |
| CSVFormat.DEFAULT.withFirstRecordAsHeader().parse(new StringReader(csvContent))) { | |
| for (CSVRecord record : parser) { | |
| if (termName.equals(record.get("name*"))) { | |
| return CSVFormat.DEFAULT.format((Object[]) record.values()); |
| await sidebarClick(page, SidebarItem.GLOSSARY); | ||
| await selectActiveGlossary(page, relGlossary.data.displayName); | ||
|
|
||
| await page.click('[data-testid="manage-button"]'); | ||
| await page.click('[data-testid="import-button-description"]'); | ||
|
|
There was a problem hiding this comment.
This test follows the same UI navigation path as earlier specs that include closeFirstPopupAlert(page) to mitigate the intermittent “glossary not found” popup under parallel execution. Consider adding that same safety step after selecting the glossary (before clicking Manage/Import) to avoid flakiness in CI.
| await sidebarClick(page, SidebarItem.GLOSSARY); | ||
| await selectActiveGlossary(page, relGlossary.data.displayName); | ||
|
|
||
| await page.click('[data-testid="manage-button"]'); | ||
| await page.click('[data-testid="import-button-description"]'); | ||
|
|
There was a problem hiding this comment.
Same navigation flake risk as the previous typed-relations spec: consider calling closeFirstPopupAlert(page) after selecting the glossary and before opening the import modal, to prevent occasional popup overlays from breaking the import flow when tests run fully parallel.
|
|
Describe your changes:
PR #25886 added typed glossary term relations and the CSV import/export plumbing to carry the
relationType:fqnformat. This PR adds test coverage only to lock that round-trip down end-to-end so the typed relations cannot silently regress to bare-FQN behavior.CsvUtilTest): 5 new tests forCsvUtil.addTermRelations()— null/empty input, defaultrelatedToprefix omission, nullrelationTypehandling, prefixed non-default types, and mixed-type alphabetic sort.GlossaryCsvRelationTypesIT): 4 strong tests — mixed-format import verified per-term via API, asymmetric relation export visible from both sides (broader/narrower), full export→re-import round-trip with three relation types preserved, and custom relation type round-trip viaGlossaryTermRelationSettings(creates + cleans up the custom type).GlossaryImportExport.spec.ts): 2 new specs — UI bulk import of CSV with mixed typed relations verifying status + API-fetched relation types + export CSV content; negative spec asserting an unknown relation type surfaces the "Invalid relation type" error in the import grid.No production code changes. Verified locally:
mvn test -pl openmetadata-service -Dtest=CsvUtilTestpasses 10/10 andmvn spotless:checkis clean across both Java modules;eslintclean on the Playwright file.Type of change:
Checklist:
🤖 Generated with Claude Code
Summary by Gitar
@Isolatedannotation toGlossaryOntologyExportITandGlossaryTermRelationITto prevent state contamination and flakes during concurrent test execution.This will update automatically on new commits.