MINOR - Data Contracts only execute tests without results#26429
MINOR - Data Contracts only execute tests without results#26429
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds first-class linkage between TestCases and Data Contracts (schema/API, indexing/search, migrations), and updates the Data Contract UI quality card to use the latest contract results while fetching relevant test cases by entity link.
Changes:
- Add
dataContractreference toTestCase+CreateTestCaseschemas/models and populate it in the service mapper/repository lifecycle. - Index
dataContractintotest_case_resultsearch documents and add filtering bydataContractIdin search/list endpoints. - Update Data Contract “Quality” UI card to use
latestContractResults.qualityValidationand fetch test cases byentityLink; extend integration test coverage for the new linkage behavior.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/generated/tests/testCase.ts | Adds dataContract?: EntityReference to generated TestCase model. |
| openmetadata-ui/src/main/resources/ui/src/generated/api/tests/createTestCase.ts | Adds dataContract?: string (FQN) to generated CreateTestCase API model. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractQualityCard/ContractQualityCard.component.tsx | Refactors quality card to fetch test cases via entity link and render summary from latest contract results. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractQualityCard/ContractQualityCard.test.tsx | Updates unit tests for the refactored quality card and new props. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractDetailTab/ContractDetail.tsx | Shows quality card based on qualityExpectations presence and passes latestContractResults. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx | Aligns tests with updated condition for rendering the quality card. |
| openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx | Minor import formatting change. |
| openmetadata-spec/src/main/resources/json/schema/tests/testCase.json | Adds dataContract entity reference to the TestCase schema. |
| openmetadata-spec/src/main/resources/json/schema/api/tests/createTestCase.json | Adds dataContract (FQN) to CreateTestCase API schema. |
| openmetadata-spec/src/main/resources/elasticsearch/en/test_case_result_index_mapping.json | Adds ES mapping for dataContract in test case result index. |
| openmetadata-spec/src/main/resources/elasticsearch/jp/test_case_result_index_mapping.json | Same as above (JP analyzers). |
| openmetadata-spec/src/main/resources/elasticsearch/ru/test_case_result_index_mapping.json | Same as above (RU analyzers). |
| openmetadata-spec/src/main/resources/elasticsearch/zh/test_case_result_index_mapping.json | Same as above (ZH analyzers). |
| openmetadata-service/src/main/java/org/openmetadata/service/search/SearchListFilter.java | Adds dataContractId filter condition for test case result searches. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/indexes/TestCaseResultIndex.java | Includes dataContract in ES docs for test case results. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResultResource.java | Accepts dataContractId query param and adds auth request context for it. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseMapper.java | Resolves CreateTestCase dataContract FQN into an EntityReference on TestCase creation. |
| openmetadata-service/src/main/java/org/openmetadata/service/migration/utils/v1130/MigrationUtil.java | Adds migration to backfill TestCase.dataContract from DataContract quality expectations. |
| openmetadata-service/src/main/java/org/openmetadata/service/migration/mysql/v1130/Migration.java | Runs the new migration in MySQL v1130 migration flow. |
| openmetadata-service/src/main/java/org/openmetadata/service/migration/postgres/v1130/Migration.java | Runs the new migration in Postgres v1130 migration flow. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataContractRepository.java | Links/unlinks test cases’ dataContract during contract create/update/delete; adjusts DQ validation/test-suite handling. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java | Adds DAO helpers to set/remove TestCase.dataContract JSON field (MySQL/Postgres). |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java | Adds integration tests for creating test cases with a data contract reference. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DataContractResourceIT.java | Adds integration tests around quality expectations/test-case linking and validation behavior. |
You can also share your feedback on Copilot code review. Take the survey.
| setIsTestCaseLoading(false); | ||
| } | ||
| }; | ||
| }, [fqn]); |
| const { qualityValidation } = latestContractResults; | ||
| const total = qualityValidation?.total ?? 0; | ||
| const success = qualityValidation?.passed ?? 0; | ||
| const failed = qualityValidation?.failed ?? 0; | ||
| const aborted = total - success - failed; | ||
|
|
||
| const successPercent = (success / total) * 100; | ||
| const failedPercent = (failed / total) * 100; | ||
| const abortedPercent = (aborted / total) * 100; | ||
|
|
||
| return { | ||
| showTestCaseSummaryChart: Boolean(total), | ||
| showTestCaseSummaryChart: true, | ||
| segmentWidths: { |
| const mergedData = contract.qualityExpectations?.map((item) => ({ | ||
| id: item.id, | ||
| name: item.name, | ||
| fullyQualifiedName: `${fqn}.${item.name}`, | ||
| testCaseStatus: | ||
| testCaseResultsMap.get(item.id)?.testCaseStatus ?? | ||
| TestCaseStatus.Queued, | ||
| })); | ||
|
|
||
| return mergedData ?? []; | ||
| }, [contract, testCase]); |
| @ConnectionAwareSqlUpdate( | ||
| value = | ||
| "UPDATE test_case SET json = JSON_REMOVE(json, '$.dataContract') WHERE id = :id AND JSON_EXTRACT(json, '$.dataContract.id') = :dataContractId", | ||
| connectionType = MYSQL) | ||
| @ConnectionAwareSqlUpdate( | ||
| value = | ||
| "UPDATE test_case SET json = json - 'dataContract' WHERE id = :id AND json->'dataContract'->>'id' = :dataContractId", | ||
| connectionType = POSTGRES) | ||
| void removeTestCaseDataContractForSpecificContract( | ||
| @Bind("id") String id, @Bind("dataContractId") String dataContractId); |
| existingValidation.withQualityScore( | ||
| (((existingValidation.getPassed() - existingValidation.getFailed()) | ||
| / (double) existingValidation.getTotal())) | ||
| * 100); |
| jest.mock('../../../utils/RouterUtils', () => ({ | ||
| getTestCaseDetailPagePath: jest.fn((fqn) => `/test-case/${fqn}`), | ||
| })); |
… and domains Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR rebases and updates the Data Contracts feature to (a) avoid triggering DQ executions when tests already have results, and (b) introduce a bidirectional dataContract reference on test cases so search/filtering and UI rendering can rely on contract-linked results.
Changes:
- Add
dataContractreference to the TestCase entity + create API, including ES mappings and search filtering bydataContractId. - Update backend contract validation/test-suite creation to split tests into “with results” vs “without results”, and backfill
dataContractreferences via v1130 migration. - Update UI
ContractQualityCardto render fromlatestContractResults.qualityValidationand fetch test cases by entity link.
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/generated/tests/testCase.ts | Adds dataContract?: EntityReference to generated TestCase type. |
| openmetadata-ui/src/main/resources/ui/src/generated/api/tests/createTestCase.ts | Adds dataContract?: string (FQN) to create-test-case request type. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractQualityCard/ContractQualityCard.component.tsx | Uses latest contract results for summary; fetches test cases via entity link. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractQualityCard/ContractQualityCard.test.tsx | Updates tests for new props/data flow and entity-link fetching. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractDetailTab/ContractDetail.tsx | Renders quality card when qualityExpectations exist and passes latest results. |
| openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractDetailTab/ContractDetail.test.tsx | Adjusts test scenario to match new render condition (quality expectations). |
| openmetadata-spec/src/main/resources/json/schema/tests/testCase.json | Adds dataContract field to TestCase JSON schema. |
| openmetadata-spec/src/main/resources/json/schema/api/tests/createTestCase.json | Adds dataContract (FQN) field to CreateTestCase schema. |
| openmetadata-spec/src/main/resources/elasticsearch/en/test_case_result_index_mapping.json | Adds dataContract mapping to test case result index (EN). |
| openmetadata-spec/src/main/resources/elasticsearch/jp/test_case_result_index_mapping.json | Adds dataContract mapping to test case result index (JP). |
| openmetadata-spec/src/main/resources/elasticsearch/ru/test_case_result_index_mapping.json | Adds dataContract mapping to test case result index (RU). |
| openmetadata-spec/src/main/resources/elasticsearch/zh/test_case_result_index_mapping.json | Adds dataContract mapping to test case result index (ZH). |
| openmetadata-service/src/main/java/org/openmetadata/service/search/SearchListFilter.java | Adds dataContractId filtering for test case result search queries. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/indexes/TestCaseResultIndex.java | Indexes dataContract into test case result search documents. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResultResource.java | Accepts dataContractId query param + includes it in auth requests & search filter. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseMapper.java | Resolves CreateTestCase dataContract FQN into an EntityReference on creation. |
| openmetadata-service/src/main/java/org/openmetadata/service/migration/utils/v1130/MigrationUtil.java | Adds v1130 data migration to backfill dataContract refs for existing test cases. |
| openmetadata-service/src/main/java/org/openmetadata/service/migration/mysql/v1130/Migration.java | Runs new v1130 backfill migration for MySQL. |
| openmetadata-service/src/main/java/org/openmetadata/service/migration/postgres/v1130/Migration.java | Runs new v1130 backfill migration for Postgres. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataContractRepository.java | Splits DQ validation path based on existing results; maintains testCase↔contract linkage. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java | Adds DB-specific JSON update/remove helpers for test case dataContract field. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java | Adds IT coverage for creating test cases with/without data contract references. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DataContractResourceIT.java | Adds IT coverage for linking/unlinking test cases via contract lifecycle + validation behavior. |
| const successPercent = (success / total) * 100; | ||
| const failedPercent = (failed / total) * 100; | ||
| const abortedPercent = (aborted / total) * 100; | ||
|
|
||
| return { | ||
| showTestCaseSummaryChart: Boolean(total), | ||
| showTestCaseSummaryChart: true, |
There was a problem hiding this comment.
successPercent/failedPercent/abortedPercent are computed by dividing by total, but showTestCaseSummaryChart is always set to true whenever qualityValidation exists. If total is 0 this produces NaN% widths and renders a broken chart. Consider returning showTestCaseSummaryChart: Boolean(total) and guarding the percentage calculations when total === 0.
| fullyQualifiedName: `${fqn}.${item.name}`, | ||
| testCaseStatus: | ||
| testCaseResultsMap.get(item.id)?.testCaseStatus ?? | ||
| TestCaseStatus.Queued, | ||
| })); | ||
|
|
||
| return mergedData ?? []; | ||
| }, [contract, testCase]); |
There was a problem hiding this comment.
fullyQualifiedName for each quality expectation is being reconstructed as ${fqn}.${item.name}. Test case FQNs are quoted/encoded in the backend when needed (e.g., special chars), and the EntityReference already carries fullyQualifiedName. Prefer using item.fullyQualifiedName (with a safe fallback) instead of rebuilding it to avoid broken links.
| fullyQualifiedName: `${fqn}.${item.name}`, | |
| testCaseStatus: | |
| testCaseResultsMap.get(item.id)?.testCaseStatus ?? | |
| TestCaseStatus.Queued, | |
| })); | |
| return mergedData ?? []; | |
| }, [contract, testCase]); | |
| fullyQualifiedName: | |
| item.fullyQualifiedName ?? (fqn ? `${fqn}.${item.name}` : item.name), | |
| testCaseStatus: | |
| testCaseResultsMap.get(item.id)?.testCaseStatus ?? | |
| TestCaseStatus.Queued, | |
| })); | |
| return mergedData ?? []; | |
| }, [contract, testCase, fqn]); |
| const mergedData = contract.qualityExpectations?.map((item) => ({ | ||
| id: item.id, | ||
| name: item.name, | ||
| fullyQualifiedName: `${fqn}.${item.name}`, | ||
| testCaseStatus: | ||
| testCaseResultsMap.get(item.id)?.testCaseStatus ?? | ||
| TestCaseStatus.Queued, | ||
| })); | ||
|
|
There was a problem hiding this comment.
Status is derived from testCaseResultsMap.get(item.id)?.testCaseStatus, but the test-case status is often represented under testCaseResult.testCaseStatus (including in existing mocks). This can cause all expectations to appear as Queued even when results exist. Consider falling back to testCaseResult?.testCaseStatus (or ensuring the search API always populates testCaseStatus).
| const mergedData = contract.qualityExpectations?.map((item) => ({ | |
| id: item.id, | |
| name: item.name, | |
| fullyQualifiedName: `${fqn}.${item.name}`, | |
| testCaseStatus: | |
| testCaseResultsMap.get(item.id)?.testCaseStatus ?? | |
| TestCaseStatus.Queued, | |
| })); | |
| const mergedData = contract.qualityExpectations?.map((item) => { | |
| const matchedTestCase = testCaseResultsMap.get(item.id); | |
| return { | |
| id: item.id, | |
| name: item.name, | |
| fullyQualifiedName: `${fqn}.${item.name}`, | |
| testCaseStatus: | |
| matchedTestCase?.testCaseStatus ?? | |
| matchedTestCase?.testCaseResult?.testCaseStatus ?? | |
| TestCaseStatus.Queued, | |
| }; | |
| }); |
| (((existingValidation.getPassed() - existingValidation.getFailed()) | ||
| / (double) existingValidation.getTotal())) | ||
| * 100); |
There was a problem hiding this comment.
Quality score calculation uses (passed - failed) / total * 100. Since passed is already the count of successful tests (and failed is the count of failed tests), subtracting failed again undercounts and can even go negative. This should be based on passed / total (or (total - failed) / total) after updating the counters.
| (((existingValidation.getPassed() - existingValidation.getFailed()) | |
| / (double) existingValidation.getTotal())) | |
| * 100); | |
| (existingValidation.getPassed() / (double) existingValidation.getTotal()) * 100); |
|
|
||
| @ConnectionAwareSqlUpdate( | ||
| value = | ||
| "UPDATE test_case SET json = JSON_REMOVE(json, '$.dataContract') WHERE id = :id AND JSON_EXTRACT(json, '$.dataContract.id') = :dataContractId", |
There was a problem hiding this comment.
MySQL branch compares JSON_EXTRACT(json, '$.dataContract.id') directly to a string bind param. JSON_EXTRACT returns a JSON value (e.g., quoted string), so this predicate may never match and the field won't be removed. Use JSON_UNQUOTE(JSON_EXTRACT(...)) = :dataContractId (pattern already used elsewhere in this file) or MySQL's ->> operator equivalent.
| "UPDATE test_case SET json = JSON_REMOVE(json, '$.dataContract') WHERE id = :id AND JSON_EXTRACT(json, '$.dataContract.id') = :dataContractId", | |
| "UPDATE test_case SET json = JSON_REMOVE(json, '$.dataContract') WHERE id = :id AND JSON_UNQUOTE(JSON_EXTRACT(json, '$.dataContract.id')) = :dataContractId", |
…tus fallback, quality score formula, MySQL JSON_UNQUOTE Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if (!nullOrEmpty(dataContract.getQualityExpectations())) { | ||
| try { | ||
| deployAndTriggerDQValidation(dataContract); | ||
| // Don't compile result yet - keep status as "Running" | ||
| // Final compilation will happen in updateContractDQResults() with complete data | ||
| } catch (Exception e) { | ||
| LOG.error( | ||
| "Failed to trigger DQ validation for data contract {}: {}", | ||
| dataContract.getFullyQualifiedName(), | ||
| e.getMessage()); | ||
| result | ||
| .withContractExecutionStatus(ContractExecutionStatus.Aborted) | ||
| .withResult("Failed to trigger DQ validation: " + e.getMessage()); | ||
| compileResult(result, ContractExecutionStatus.Aborted); | ||
| // Check if all tests already have results | ||
| List<TestCase> tests = getTestsWithResults(dataContract); | ||
| List<TestCase> testsWithoutResults = filterTestsWithoutResults(tests); | ||
| List<TestCase> testsWithResults = filterTestsWithResults(tests); | ||
|
|
||
| // Initialize the quality validation results | ||
| result.withQualityValidation(initDQValidation(dataContract)); | ||
|
|
||
| if (!nullOrEmpty(testsWithoutResults)) { | ||
| try { | ||
| deployAndTriggerDQValidation(dataContract); | ||
| } catch (Exception e) { | ||
| LOG.error( | ||
| "Failed to trigger DQ validation for data contract {}: {}", | ||
| dataContract.getFullyQualifiedName(), | ||
| e.getMessage()); | ||
| result | ||
| .withContractExecutionStatus(ContractExecutionStatus.Aborted) | ||
| .withResult("Failed to trigger DQ validation: " + e.getMessage()); | ||
| compileResult(result, ContractExecutionStatus.Aborted); | ||
| } | ||
| } | ||
| if (!nullOrEmpty(testsWithResults)) { | ||
| QualityValidation qualityValidation = | ||
| getExistingTestResults(dataContract, testsWithResults); | ||
| result.withQualityValidation(qualityValidation); | ||
| // Fallback to running if we're still waiting to some tests to report back their status | ||
| compileResult( | ||
| result, | ||
| !nullOrEmpty(testsWithoutResults) | ||
| ? ContractExecutionStatus.Running | ||
| : ContractExecutionStatus.Success); | ||
| } |
There was a problem hiding this comment.
In validateContract(...), when there are qualityExpectations and all referenced tests are missing results (testsWithoutResults non-empty, testsWithResults empty), the code triggers deployAndTriggerDQValidation(...) but never calls compileResult(...). That leaves the contract status as Running even if schema/semantics validations already failed (since compileResult is what flips status to Failed). Consider calling compileResult(result, ContractExecutionStatus.Running) after initializing qualityValidation / triggering the pipeline so schema/semantics failures are reflected immediately.
OpenMetadata Service New-Code Coverage❌ FAIL. Required changed-line coverage:
Files below threshold:
Only changed executable lines under |
…ractRepository, MigrationUtil, TestCaseResultIndex) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| const mergedData = contract.qualityExpectations?.map((item) => { | ||
| const matchedTestCase = testCaseResultsMap.get(item.id); | ||
|
|
||
| return { | ||
| id: item.id, | ||
| name: item.name, | ||
| fullyQualifiedName: | ||
| item.fullyQualifiedName ?? (fqn ? `${fqn}.${item.name}` : item.name), | ||
| testCaseStatus: | ||
| matchedTestCase?.testCaseStatus ?? | ||
| matchedTestCase?.testCaseResult?.testCaseStatus ?? | ||
| TestCaseStatus.Queued, | ||
| }; |
There was a problem hiding this comment.
processedQualityExpectations builds a fallback fullyQualifiedName as ${fqn}.${item.name} when the EntityReference doesn't include fullyQualifiedName. That can produce incorrect/unresolvable TestCase FQNs (especially if names contain spaces/special chars), and you already have the resolved TestCase from the search response. Prefer item.fullyQualifiedName ?? matchedTestCase?.fullyQualifiedName (and only fall back to string concatenation if you're sure that's the canonical TestCase FQN format).
| SdkClients.adminClient() | ||
| .dataContracts() | ||
| .get(contract.getId().toString(), "testSuite,qualityExpectations"); | ||
|
|
There was a problem hiding this comment.
The test testCreateContractWithQualityExpectations_TestSuiteCreated doesn't assert that a test suite was actually created (it only checks qualityExpectations). Either add an assertion on fetched.getTestSuite() (and optionally its tests/pipelines), or rename the test to reflect what it verifies.
| assertNotNull(fetched.getTestSuite(), "Test suite should be created when quality expectations are defined"); |
| // Fallback to running if we're still waiting to some tests to report back their status | ||
| compileResult( | ||
| result, | ||
| !nullOrEmpty(testsWithoutResults) | ||
| ? ContractExecutionStatus.Running | ||
| : ContractExecutionStatus.Success); |
There was a problem hiding this comment.
In validateContract, when some tests already have results and others are still pending execution (testsWithoutResults not empty), calling compileResult(..., Running) can still flip the status to Failed if any existing result failed. That makes the returned result look finished (not Running) even though the DQ pipeline was triggered, and it also prevents abortRunningValidation() from aborting subsequent validations while the pipeline is still running. Consider skipping compileResult while there are pending tests (keep status Running explicitly) and only compile to Success/Failed once all expectations have a terminal status.
| // Fallback to running if we're still waiting to some tests to report back their status | |
| compileResult( | |
| result, | |
| !nullOrEmpty(testsWithoutResults) | |
| ? ContractExecutionStatus.Running | |
| : ContractExecutionStatus.Success); | |
| // Only compile the final result when all tests have reported their status. | |
| // If there are still tests without results, keep the execution status as RUNNING | |
| // and let the subsequent updates (when tests finish) compile the final outcome. | |
| if (nullOrEmpty(testsWithoutResults)) { | |
| compileResult(result, ContractExecutionStatus.Success); | |
| } else { | |
| result.withContractExecutionStatus(ContractExecutionStatus.Running); | |
| } |
| @@ -1223,14 +1353,14 @@ private QualityValidation validateDQ(TestSuite testSuite) { | |||
| List<ResultSummary> failedTests = | |||
| testSummary.stream().filter(test -> FAILED_DQ_STATUSES.contains(test.getStatus())).toList(); | |||
|
|
|||
| validation | |||
| .withFailed(failedTests.size()) | |||
| .withPassed(testSummary.size() - failedTests.size()) | |||
| .withTotal(testSummary.size()) | |||
| .withQualityScore( | |||
| (((testSummary.size() - failedTests.size()) / (double) testSummary.size())) * 100); | |||
| existingValidation | |||
| .withFailed(existingValidation.getFailed() + failedTests.size()) | |||
| .withPassed(existingValidation.getPassed() + (testSummary.size() - failedTests.size())); | |||
|
|
|||
| return validation; | |||
| existingValidation.withQualityScore( | |||
| (existingValidation.getPassed() / (double) existingValidation.getTotal()) * 100); | |||
|
|
|||
There was a problem hiding this comment.
validateDQ mutates existingValidation by adding to getFailed()/getPassed() and then divides by getTotal(). This can throw NPE if existingValidation exists but has null counts/total (e.g., results created via validateContractWithEffective don’t initialize qualityValidation, or older stored results). It can also over-count on subsequent test suite executions by repeatedly adding the same suite summary. Consider normalizing null counts to 0, ensuring total is set before computing the score, and recomputing the DQ portion from the current testSuite summary (instead of incrementing) so repeated executions don’t inflate passed/failed.
| }); | ||
|
|
||
| it('should not display test cases when contract has quality expectations', async () => { | ||
| it("should not display test cases when contract doesn't have quality expectations", async () => { |
There was a problem hiding this comment.
The new test name uses double quotes while the surrounding tests in this file use single quotes. Aligning the quote style keeps the test file consistent and avoids unnecessary diffs.
| it("should not display test cases when contract doesn't have quality expectations", async () => { | |
| it('should not display test cases when contract does not have quality expectations', async () => { |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d-review-pr-23335-d42f1f3d # Conflicts: # openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DataContractResourceIT.java # openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SearchListFilterTest.java
| const { t } = useTranslation(); | ||
| const { fqn } = useFqn(); | ||
| const [isTestCaseLoading, setIsTestCaseLoading] = useState(false); | ||
| const [testCaseSummary, setTestCaseSummary] = useState<TestSummary>(); | ||
| const [testCaseResult, setTestCaseResult] = useState<TestCase[]>([]); | ||
| const [testCase, setTestCase] = useState<TestCase[]>([]); | ||
|
|
||
| const fetchTestCaseSummary = async () => { | ||
| try { | ||
| const response = await getTestCaseExecutionSummary( | ||
| contract?.testSuite?.id | ||
| ); | ||
| setTestCaseSummary(response); | ||
| } catch { | ||
| // silent fail | ||
| const fetchTestCases = useCallback(async () => { | ||
| if (!fqn) { | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| const fetchTestCases = async () => { | ||
| setIsTestCaseLoading(true); | ||
| try { | ||
| const response = await getListTestCaseBySearch({ | ||
| testSuiteId: contract?.testSuite?.id, | ||
| const { data } = await getListTestCaseBySearch({ | ||
| ...DEFAULT_SORT_ORDER, | ||
| entityLink: generateEntityLink(fqn), | ||
| includeAllTests: true, | ||
| limit: ES_MAX_PAGE_SIZE, | ||
| include: Include.NonDeleted, | ||
| }); |
There was a problem hiding this comment.
fetchTestCases is building entityLink from the route fqn via generateEntityLink(fqn), but useFqn() returns the data contract FQN for this page, not the referenced entity’s FQN. Also, generateEntityLink always produces a table entity-link (<#E::table::...>), so this will query the wrong test cases for non-table contracts and even for table contracts it will use the contract FQN instead of contract.entity.fullyQualifiedName. Use contract.entity (type + FQN) to build the entityLink (or a generic entity-link util) rather than useFqn + table-only helper.
There was a problem hiding this comment.
Fixed — now using contract.entity?.fullyQualifiedName instead of the route FQN from useFqn(). Removed the useFqn dependency entirely from this component.
| const mergedData = contract.qualityExpectations?.map((item) => { | ||
| const matchedTestCase = testCaseResultsMap.get(item.id); | ||
|
|
||
| return { | ||
| id: item.id, | ||
| name: item.name, | ||
| fullyQualifiedName: | ||
| item.fullyQualifiedName ?? (fqn ? `${fqn}.${item.name}` : item.name), | ||
| testCaseStatus: | ||
| matchedTestCase?.testCaseStatus ?? | ||
| matchedTestCase?.testCaseResult?.testCaseStatus ?? | ||
| TestCaseStatus.Queued, | ||
| }; |
There was a problem hiding this comment.
The fallback fullyQualifiedName computation (${fqn}.${item.name}) can generate invalid test-case FQNs (and currently uses the contract route FQN). Since navigation relies on a valid test-case FQN, it’s safer to require item.fullyQualifiedName (or derive it from the fetched TestCase record) rather than guessing it from item.name.
There was a problem hiding this comment.
Fixed — removed the ${fqn}.${item.name} fallback. Now uses item.fullyQualifiedName ?? item.name directly from the EntityReference.
| '/test-case/fqn.CLV Must be Positive' | ||
| ); | ||
| expect(links[1]).toHaveAttribute( | ||
| 'href', | ||
| '/test-case/fqn.Customer ID To Be Unique' | ||
| ); | ||
| expect(links[2]).toHaveAttribute( | ||
| 'href', | ||
| '/test-case/fqn.Table Row Count To Equal' |
There was a problem hiding this comment.
These link expectations don’t match the actual route building: getTestCaseDetailPagePath URL-encodes the FQN via encodeURIComponent, so spaces (and other chars) will be encoded (e.g. %20). Update the assertions to match the encoded hrefs produced by getTestCaseDetailPagePath.
| '/test-case/fqn.CLV Must be Positive' | |
| ); | |
| expect(links[1]).toHaveAttribute( | |
| 'href', | |
| '/test-case/fqn.Customer ID To Be Unique' | |
| ); | |
| expect(links[2]).toHaveAttribute( | |
| 'href', | |
| '/test-case/fqn.Table Row Count To Equal' | |
| '/test-case/fqn.CLV%20Must%20be%20Positive' | |
| ); | |
| expect(links[1]).toHaveAttribute( | |
| 'href', | |
| '/test-case/fqn.Customer%20ID%20To%20Be%20Unique' | |
| ); | |
| expect(links[2]).toHaveAttribute( | |
| 'href', | |
| '/test-case/fqn.Table%20Row%20Count%20To%20Equal' |
There was a problem hiding this comment.
Not applicable — the test mocks getTestCaseDetailPagePath at line 68 (jest.fn((fqn) => \/test-case/${fqn}``), so URL encoding does not apply here. The test validates component rendering logic, not the real router utility.
| // If we don't have quality expectations, flag the results based on schema and semantics | ||
| // Otherwise, keep it Running and wait for the DQ results to kick in | ||
| // Otherwise, check if we need to run tests or use existing results | ||
| if (!nullOrEmpty(dataContract.getQualityExpectations())) { | ||
| try { | ||
| deployAndTriggerDQValidation(dataContract); | ||
| // Don't compile result yet - keep status as "Running" | ||
| // Final compilation will happen in updateContractDQResults() with complete data | ||
| } catch (Exception e) { | ||
| LOG.error( | ||
| "Failed to trigger DQ validation for data contract {}: {}", | ||
| dataContract.getFullyQualifiedName(), | ||
| e.getMessage()); | ||
| result | ||
| .withContractExecutionStatus(ContractExecutionStatus.Aborted) | ||
| .withResult("Failed to trigger DQ validation: " + e.getMessage()); | ||
| compileResult(result, ContractExecutionStatus.Aborted); | ||
| // Check if all tests already have results | ||
| List<TestCase> tests = getTestsWithResults(dataContract); | ||
| List<TestCase> testsWithoutResults = filterTestsWithoutResults(tests); | ||
| List<TestCase> testsWithResults = filterTestsWithResults(tests); | ||
|
|
||
| // Initialize the quality validation results | ||
| result.withQualityValidation(initDQValidation(dataContract)); | ||
|
|
||
| if (!nullOrEmpty(testsWithoutResults)) { | ||
| try { | ||
| deployAndTriggerDQValidation(dataContract); | ||
| } catch (Exception e) { | ||
| LOG.error( | ||
| "Failed to trigger DQ validation for data contract {}: {}", | ||
| dataContract.getFullyQualifiedName(), | ||
| e.getMessage()); | ||
| result | ||
| .withContractExecutionStatus(ContractExecutionStatus.Aborted) | ||
| .withResult("Failed to trigger DQ validation: " + e.getMessage()); | ||
| compileResult(result, ContractExecutionStatus.Aborted); | ||
| } | ||
| } | ||
| if (!nullOrEmpty(testsWithResults)) { | ||
| QualityValidation qualityValidation = | ||
| getExistingTestResults(dataContract, testsWithResults); | ||
| result.withQualityValidation(qualityValidation); | ||
| // Fallback to running if we're still waiting to some tests to report back their status | ||
| compileResult( | ||
| result, | ||
| !nullOrEmpty(testsWithoutResults) | ||
| ? ContractExecutionStatus.Running | ||
| : ContractExecutionStatus.Success); | ||
| } |
There was a problem hiding this comment.
If a contract has quality expectations but only testsWithoutResults (no existing results yet), compileResult is never called. That means schema/semantics failures won’t be reflected in contractExecutionStatus (it will remain Running even when schema/semantics already failed). Consider calling compileResult(result, ContractExecutionStatus.Running) after triggering DQ so schema/semantics failures are still surfaced while DQ is pending.
There was a problem hiding this comment.
Fixed — added compileResult(result, ContractExecutionStatus.Running) when only testsWithoutResults exist and DQ trigger succeeded, so schema/semantics failures are surfaced while DQ is pending. Skipped when status is already Aborted (DQ trigger failed).
| @@ -1231,14 +1361,16 @@ private QualityValidation validateDQ(TestSuite testSuite) { | |||
| List<ResultSummary> failedTests = | |||
| testSummary.stream().filter(test -> FAILED_DQ_STATUSES.contains(test.getStatus())).toList(); | |||
|
|
|||
| validation | |||
| .withFailed(failedTests.size()) | |||
| .withPassed(testSummary.size() - failedTests.size()) | |||
| .withTotal(testSummary.size()) | |||
| .withQualityScore( | |||
| (((testSummary.size() - failedTests.size()) / (double) testSummary.size())) * 100); | |||
| existingValidation | |||
| .withFailed(existingValidation.getFailed() + failedTests.size()) | |||
| .withPassed(existingValidation.getPassed() + (testSummary.size() - failedTests.size())); | |||
|
|
|||
| return validation; | |||
| existingValidation.withQualityScore( | |||
| existingValidation.getTotal() > 0 | |||
| ? (existingValidation.getPassed() / (double) existingValidation.getTotal()) * 100 | |||
| : 0.0); | |||
|
|
|||
There was a problem hiding this comment.
validateDQ can throw NPE when existingValidation comes from older persisted results where total, passed, or failed are null. The code does arithmetic/comparisons on existingValidation.getFailed()/getPassed()/getTotal() without null defaults. Initialize missing fields to 0 (and set total if absent) before accumulating and computing qualityScore.
There was a problem hiding this comment.
Fixed — validateDQ now initializes a fresh QualityValidation with explicit zeros and uses null-safe local variables (priorFailed, priorPassed, total) before accumulating.
| @@ -243,6 +251,8 @@ protected void postDelete(DataContract dataContract, boolean hardDelete) { | |||
| if (!nullOrEmpty(dataContract.getQualityExpectations())) { | |||
| deleteTestSuite(dataContract); | |||
| } | |||
| // Remove dataContract reference from all associated test cases | |||
| removeDataContractFromTestCases(dataContract); | |||
| // Clean status | |||
There was a problem hiding this comment.
removeDataContractFromTestCases returns early when qualityExpectations is empty. However, a test case can now be created with a dataContract reference via CreateTestCase.dataContract/TestCaseMapper even if the contract has no qualityExpectations. Deleting such a contract will leave dangling testCase.dataContract references. Consider removing references by contract id (e.g., update all test_case rows where json->dataContract->id matches) rather than iterating only qualityExpectations.
There was a problem hiding this comment.
Fixed — added removeAllTestCaseDataContractReferences(dataContractId) DAO method that bulk-removes dataContract references from all test cases matching the contract ID. removeDataContractFromTestCases now uses this instead of iterating qualityExpectations, handling both paths (qualityExpectations and CreateTestCase.dataContract).
The method isSupportedEntityType was renamed to isEntityTypeSupported and made public, causing NoSuchMethod errors and mock interception failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
…refs - Use contract.entity.fullyQualifiedName instead of route FQN for test case entityLink query (fixes wrong test cases for non-table contracts) - Remove invalid FQN fallback using contract route FQN - Add null-safety in validateDQ for QualityValidation fields - Call compileResult when only testsWithoutResults exist so schema/semantics failures are surfaced while DQ is pending - Add bulk DAO method to remove dataContract refs by contract ID, preventing dangling references on contract deletion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
| expect(getListTestCaseBySearch).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| testSuiteId: MOCK_DATA_CONTRACT.testSuite.id, | ||
| entityLink: '<#E::table::fqn>', | ||
| include: 'non-deleted', | ||
| includeAllTests: true, | ||
| limit: 10000, | ||
| sortField: 'testCaseResult.timestamp', | ||
| sortType: 'desc', | ||
| }) |
There was a problem hiding this comment.
The expectation for entityLink is hardcoded to <#E::table::fqn>, but the component builds it from contract.entity.fullyQualifiedName via generateEntityLink(entityFqn). With the current MOCK_DATA_CONTRACT (entity FQN redshift prod.dev.dbt_jaffle.customers), this assertion will fail. Update the test to derive the expected entityLink from the mock contract (or mock generateEntityLink to return a stable value) so it matches the component behavior.
| return dataContract.getQualityExpectations().stream() | ||
| .map( | ||
| testRef -> { | ||
| try { | ||
| return testCaseRepository.get( | ||
| null, testRef.getId(), new Fields(Set.of(TEST_CASE_RESULT))); | ||
| } catch (EntityNotFoundException e) { | ||
| LOG.warn("Test case {} not found: {}", testRef.getId(), e.getMessage()); | ||
| return null; | ||
| } | ||
| }) | ||
| .filter(Objects::nonNull) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
getTestsWithResults() fetches each test case individually via testCaseRepository.get(...) for every quality expectation. This introduces an N+1 pattern during contract create/update and validation (and can be called multiple times per request path), which may become a bottleneck for contracts with many test cases. Consider using a bulk DAO/repository fetch (batch by IDs) or reusing an already-fetched list within the same operation to avoid repeated per-test queries.
| return dataContract.getQualityExpectations().stream() | |
| .map( | |
| testRef -> { | |
| try { | |
| return testCaseRepository.get( | |
| null, testRef.getId(), new Fields(Set.of(TEST_CASE_RESULT))); | |
| } catch (EntityNotFoundException e) { | |
| LOG.warn("Test case {} not found: {}", testRef.getId(), e.getMessage()); | |
| return null; | |
| } | |
| }) | |
| .filter(Objects::nonNull) | |
| .collect(Collectors.toList()); | |
| // Collect unique, non-null test case IDs from the quality expectations | |
| List<UUID> testCaseIds = | |
| dataContract.getQualityExpectations().stream() | |
| .map(EntityReference::getId) | |
| .filter(Objects::nonNull) | |
| .distinct() | |
| .collect(Collectors.toList()); | |
| if (testCaseIds.isEmpty()) { | |
| return Collections.emptyList(); | |
| } | |
| // Bulk-fetch all test cases with their latest results | |
| List<TestCase> fetchedTests = | |
| testCaseRepository.getByIds(testCaseIds, new Fields(Set.of(TEST_CASE_RESULT))); | |
| // Index fetched tests by ID for quick lookup | |
| Map<UUID, TestCase> testsById = | |
| fetchedTests.stream() | |
| .filter(Objects::nonNull) | |
| .collect(Collectors.toMap(TestCase::getId, test -> test, (left, right) -> left)); | |
| // Preserve original order from quality expectations and log missing tests | |
| List<TestCase> result = new ArrayList<>(); | |
| dataContract.getQualityExpectations().forEach( | |
| testRef -> { | |
| if (testRef.getId() == null) { | |
| return; | |
| } | |
| TestCase test = testsById.get(testRef.getId()); | |
| if (test == null) { | |
| LOG.warn("Test case {} not found during bulk fetch", testRef.getId()); | |
| return; | |
| } | |
| result.add(test); | |
| }); | |
| return result; |
| it('should render loader when data is loading', async () => { | ||
| render( | ||
| <MemoryRouter> | ||
| <ContractQualityCard contract={MOCK_DATA_CONTRACT} /> | ||
| </MemoryRouter> | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByTestId('loader')).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The loader test doesn't set up getListTestCaseBySearch to return a pending promise. Since the mock currently returns undefined, await getListTestCaseBySearch(...) resolves immediately and the destructuring throws, causing the finally to set isTestCaseLoading back to false in the same tick. With React 18 batching this can make the loader render non-deterministic/flaky. Make the mock return a never-resolving promise (or a controllable deferred) for this test so the loading state remains true while asserting the loader.
|
Summary
Rebases and resolves conflicts for PR #23335 (
dc-dq-execute-summary) against currentmain.Core changes from original PR:
dataContractfield totestCase.jsonschema so test cases can reference their parent contractsdataContractreferences on existing test casesContractQualityCardto uselatestContractResults.qualityValidationprop instead of fetching test suite summary separatelyCollectionDAO.javafor managing test case references (PostgreSQLjsonb_setvs MySQLJSON_SET)dataContractIdinTestCaseResultResourceConflict resolutions:
MigrationWorkflow.loadMigrations()) + added the newmigrateTestCaseDataContractReferencescallwithTopDimensionsfrom main with thedataContractconditional set from the PR!isEmpty(contract?.qualityExpectations)condition and passlatestContractResultspropqualityExpectationscondition with main's additionalentityId/entityTypepropsAdditional fix:
test_case_result_index_mapping.json(was broken in the original PR branch due to structural issues)Test plan
ContractQualityCardrenders quality validation data from contract resultsdataContractreference worksdataContractreferences on existing test casesmvn spotless:applyto ensure Java formatting