MINOR - Data Contracts only execute tests without results#26429
MINOR - Data Contracts only execute tests without results#26429
Conversation
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>
| 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.
… v2.0.0 Conflicts resolved by keeping both sides (independent additions). Moved migrateTestCaseDataContractReferences from v1130 to v200 since 1.13 is already released. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
…act/ContractDetailTab/ContractDetail.test.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Merge PR #23335 changes onto main with migration moved to v2.0.0. Changes: - Data contracts now only execute tests that don't already have results - Add dataContract reference to test cases for filtering - Add dataContractId filter to test case result search API - Move migrateTestCaseDataContractReferences to v2.0.0 migration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hub.com:open-metadata/OpenMetadata into task/fix-conflicts-and-review-pr-23335-d42f1f3d
|
|
||
| jest.mock('../../../rest/testAPI', () => ({ | ||
| getTestCaseExecutionSummary: jest.fn(), | ||
| getListTestCaseBySearch: jest.fn(), |
There was a problem hiding this comment.
The getListTestCaseBySearch mock has no default resolved value. In tests that render the component without explicitly setting mockResolvedValue (e.g., the loader test), the component will await the mock, destructure { data }, and can throw because the mock returns undefined by default. Consider setting a safe default (e.g., resolving to { data: [] }) and overriding per-test when needed.
| getListTestCaseBySearch: jest.fn(), | |
| getListTestCaseBySearch: jest.fn().mockResolvedValue({ data: [] }), |
| await waitFor(() => { | ||
| const links = screen.getAllByRole('link'); | ||
|
|
||
| expect(links[0]).toHaveAttribute('href', '/test-case/table.test_case_1'); | ||
| expect(links[1]).toHaveAttribute('href', '/test-case/table.test_case_2'); | ||
| expect(links[2]).toHaveAttribute('href', '/test-case/table.test_case_3'); | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle test cases with missing test results', async () => { | ||
| const testCasesWithoutResults: TestCase[] = [ | ||
| { | ||
| id: 'test-case-4', | ||
| name: 'Test Case 4', | ||
| fullyQualifiedName: 'table.test_case_4', | ||
| } as TestCase, | ||
| ]; | ||
|
|
||
| (getTestCaseExecutionSummary as jest.Mock).mockResolvedValue( | ||
| mockTestSummary | ||
| ); | ||
| (getListTestCaseBySearch as jest.Mock).mockResolvedValue({ | ||
| data: testCasesWithoutResults, | ||
| }); | ||
|
|
||
| render( | ||
| <MemoryRouter> | ||
| <ContractQualityCard contract={MOCK_DATA_CONTRACT} /> | ||
| </MemoryRouter> | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Test Case 4')).toBeInTheDocument(); | ||
| expect(links[0]).toHaveAttribute( | ||
| 'href', | ||
| '/test-case/CLV Must be Positive' | ||
| ); | ||
| expect(links[1]).toHaveAttribute( | ||
| 'href', | ||
| '/test-case/Customer ID To Be Unique' | ||
| ); | ||
| expect(links[2]).toHaveAttribute( | ||
| 'href', | ||
| '/test-case/Table Row Count To Equal' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This link assertion expects navigation to use the test case display/name (with spaces). The UI route helper getTestCaseDetailPagePath is designed for the test case FQN (which is derived from the entity FQN + quoted test name). To avoid masking regressions, update the test data so MOCK_DATA_CONTRACT.qualityExpectations and mockTestCases share the same IDs and the component can resolve matchedTestCase.fullyQualifiedName, then assert the href uses that FQN (encoded).
| @@ -243,6 +251,8 @@ protected void postDelete(DataContract dataContract, boolean hardDelete) { | |||
| if (!nullOrEmpty(dataContract.getQualityExpectations())) { | |||
There was a problem hiding this comment.
postDelete deletes the test suite whenever qualityExpectations is non-empty, but deleteTestSuite() calls getOrCreateTestSuite(). With the new logic that can clear dataContract.testSuite when no tests are pending, this path can end up creating a new logical test suite during deletion just to delete it. Gate this on contractHasTestSuite(dataContract) (or make deleteTestSuite only delete an existing suite by reference/name without creating one).
| if (!nullOrEmpty(dataContract.getQualityExpectations())) { | |
| if (!nullOrEmpty(dataContract.getQualityExpectations()) | |
| && dataContract.getTestSuite() != null) { |
| private void updateTestSuiteTests( | ||
| DataContract dataContract, TestSuite testSuite, List<TestCase> testsWithoutResults) { | ||
| TestCaseRepository testCaseRepository = | ||
| (TestCaseRepository) Entity.getEntityRepository(Entity.TEST_CASE); | ||
|
|
There was a problem hiding this comment.
In updateTestSuiteTests(...), testCaseRefs is still computed from dataContract.getQualityExpectations() but no longer used after switching removal logic to testsToAdd. Please remove the unused local (or use it) to avoid confusion about which source of truth drives add/remove behavior.
| daoCollection | ||
| .testCaseDAO() | ||
| .updateTestCaseDataContract( | ||
| testCaseRef.getId().toString(), JsonUtils.pojoToJson(dataContractRef)); | ||
|
|
There was a problem hiding this comment.
updateTestCasesWithDataContract updates test_case.json directly via updateTestCaseDataContract(...) (JSON_SET/jsonb_set). This bypasses the normal TestCaseRepository update flow (versioning/updatedAt+updatedBy tracking, change events, and any search-index update hooks). If the intent is to keep the back-reference in sync for UI/search, consider performing an entity update through the repository/updater (or otherwise ensuring downstream indexing/auditing stays consistent).
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
|
|
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 formattingSummary by Gitar
entity_extensionversioning, field-change tracking, and relationship count queries.FeedDAOenhancements to supportTask,Announcement, andActivityStreamoperations.v200migrations to handleTaskentity migration, legacy activity thread conversion, and announcement relationship backfilling.executeWithDeadlockRetryinTestCaseResourceITto mitigate transient database deadlocks during entity updates.Awaitilitytimeouts from 30s to 90s-180s in integration tests to reduce flakiness.autoCloseIncidentboolean field totestCase.jsonto support automatic incident resolution.This will update automatically on new commits.