Skip to content

Commit fd32a7c

Browse files
CopilotShaileshParmar11
Copilot
and
ShaileshParmar11
committed
Fix: Boolean parameters in test case forms cannot be toggled OFF (#25918)
* Initial plan * Fix matchEnum toggle issue in test case forms Convert boolean parameter values from string to boolean when loading test case data for editing. This fixes the issue where the matchEnum switch couldn't be turned OFF once enabled. Co-authored-by: ShaileshParmar11 <71748675+ShaileshParmar11@users.noreply.github.com> * Add unit tests for boolean parameter conversion Add mock data and test cases to verify that boolean parameters are correctly converted from string to boolean when loading test case data. Co-authored-by: ShaileshParmar11 <71748675+ShaileshParmar11@users.noreply.github.com> * Improve unit tests to verify switch state Update tests to check the actual switch element aria-checked attribute to properly validate that boolean parameters are correctly converted to their boolean values. Co-authored-by: ShaileshParmar11 <71748675+ShaileshParmar11@users.noreply.github.com> * Address code review feedback for tests - Move mock imports to top of test file - Make tests fail explicitly if switch element is not found - Remove conditional assertions that could hide test failures Co-authored-by: ShaileshParmar11 <71748675+ShaileshParmar11@users.noreply.github.com> * Apply boolean parameter fix to EditTestCaseModal.tsx Apply the same boolean parameter conversion fix to the non-V1 version of EditTestCaseModal. This ensures the matchEnum toggle works correctly in all places where test cases are edited, including TestCaseResultTab and IncidentManagerDetailPage. Co-authored-by: ShaileshParmar11 <71748675+ShaileshParmar11@users.noreply.github.com> * test: enhance Column Level Data Quality tests and improve boolean parameter handling in EditTestCaseModal * test: add boolean parameter handling and improve JSON validation in TestCaseForm --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ShaileshParmar11 <71748675+ShaileshParmar11@users.noreply.github.com> Co-authored-by: Shailesh Parmar <shailesh.parmar.webdev@gmail.com>
1 parent e042bc1 commit fd32a7c

8 files changed

Lines changed: 490 additions & 145 deletions

File tree

‎openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DataQuality/ColumnLevelTests.spec.ts‎

Lines changed: 99 additions & 130 deletions
Large diffs are not rendered by default.

‎openmetadata-ui/src/main/resources/ui/src/components/DataQuality/AddDataQualityTest/EditTestCaseModal.test.tsx‎

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import { forwardRef } from 'react';
2222
import { LabelType, State, TagSource } from '../../../generated/type/tagLabel';
2323
import {
2424
MOCK_TEST_CASE,
25+
MOCK_TEST_CASE_WITH_BOOLEAN_PARAM,
26+
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_BE_IN_SET,
2527
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_MATCH_REGEX,
2628
} from '../../../mocks/TestSuite.mock';
2729
import { getTestDefinitionById } from '../../../rest/testAPI';
@@ -47,7 +49,43 @@ jest.mock('../../common/RichTextEditor/RichTextEditor', () => {
4749
);
4850
});
4951
jest.mock('./components/ParameterForm', () => {
50-
return jest.fn().mockImplementation(() => <div>ParameterForm.component</div>);
52+
const { Form } = require('antd');
53+
54+
function ParameterFormMock({
55+
definition,
56+
}: Readonly<{
57+
definition?: {
58+
parameterDefinition?: Array<{ name: string; dataType: string }>;
59+
};
60+
}>) {
61+
let params: Record<string, unknown> = {};
62+
63+
try {
64+
const form = Form.useFormInstance();
65+
params = form.getFieldValue('params') ?? {};
66+
} catch {
67+
// no form context
68+
}
69+
70+
const booleanParams = (definition?.parameterDefinition ?? []).filter(
71+
(param) => param.dataType === 'BOOLEAN'
72+
);
73+
74+
return (
75+
<div>
76+
ParameterForm.component
77+
{booleanParams.map((param) => (
78+
<button
79+
aria-checked={params[param.name] ? 'true' : 'false'}
80+
key={param.name}
81+
role="switch"
82+
/>
83+
))}
84+
</div>
85+
);
86+
}
87+
88+
return jest.fn().mockImplementation(ParameterFormMock);
5189
});
5290
jest.mock('../../../pages/TasksPage/shared/TagSuggestion', () =>
5391
jest.fn().mockImplementation(({ children, ...props }) => (
@@ -590,4 +628,68 @@ describe('EditTestCaseModal Component', () => {
590628
// Should work normally without tier tags
591629
expect(mockProps.onUpdate).toHaveBeenCalled();
592630
});
631+
632+
it('should convert boolean parameter string "false" to boolean false', async () => {
633+
(getTestDefinitionById as jest.Mock).mockResolvedValue(
634+
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_BE_IN_SET
635+
);
636+
637+
const testCaseWithBooleanFalse = {
638+
...MOCK_TEST_CASE_WITH_BOOLEAN_PARAM,
639+
parameterValues: [
640+
{ name: 'allowedValues', value: '["active","inactive"]' },
641+
{ name: 'matchEnum', value: 'false' },
642+
],
643+
};
644+
645+
render(
646+
<EditTestCaseModal {...mockProps} testCase={testCaseWithBooleanFalse} />
647+
);
648+
649+
await waitFor(() => {
650+
expect(screen.getByTestId('edit-test-form')).toBeInTheDocument();
651+
});
652+
653+
const switchElement = await waitFor(() => {
654+
const el = document.querySelector('button[role="switch"]');
655+
656+
expect(el).toBeInTheDocument();
657+
658+
return el as HTMLButtonElement;
659+
});
660+
661+
expect(switchElement.getAttribute('aria-checked')).toBe('false');
662+
});
663+
664+
it('should convert boolean parameter string "true" to boolean true', async () => {
665+
(getTestDefinitionById as jest.Mock).mockResolvedValue(
666+
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_BE_IN_SET
667+
);
668+
669+
const testCaseWithBooleanTrue = {
670+
...MOCK_TEST_CASE_WITH_BOOLEAN_PARAM,
671+
parameterValues: [
672+
{ name: 'allowedValues', value: '["active","inactive"]' },
673+
{ name: 'matchEnum', value: 'true' },
674+
],
675+
};
676+
677+
render(
678+
<EditTestCaseModal {...mockProps} testCase={testCaseWithBooleanTrue} />
679+
);
680+
681+
await waitFor(() => {
682+
expect(screen.getByTestId('edit-test-form')).toBeInTheDocument();
683+
});
684+
685+
const switchElement = await waitFor(() => {
686+
const el = document.querySelector('button[role="switch"]');
687+
688+
expect(el).toBeInTheDocument();
689+
690+
return el as HTMLButtonElement;
691+
});
692+
693+
expect(switchElement.getAttribute('aria-checked')).toBe('true');
694+
});
593695
});

‎openmetadata-ui/src/main/resources/ui/src/components/DataQuality/AddDataQualityTest/EditTestCaseModal.tsx‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,13 @@ const EditTestCaseModal: React.FC<EditTestCaseModalProps> = ({
212212
};
213213
}
214214

215+
if (param?.dataType === TestDataType.Boolean) {
216+
return {
217+
...acc,
218+
[curr.name || '']: curr.value === 'true',
219+
};
220+
}
221+
215222
return {
216223
...acc,
217224
[curr.name || '']: curr.value,

‎openmetadata-ui/src/main/resources/ui/src/components/DataQuality/AddDataQualityTest/components/EditTestCaseModalV1.test.tsx‎

Lines changed: 116 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import {
1919
} from '../../../../generated/type/tagLabel';
2020
import {
2121
MOCK_TEST_CASE,
22+
MOCK_TEST_CASE_WITH_BOOLEAN_PARAM,
23+
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_BE_IN_SET,
2224
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_MATCH_REGEX,
2325
} from '../../../../mocks/TestSuite.mock';
2426
import { getTestDefinitionById } from '../../../../rest/testAPI';
@@ -45,7 +47,43 @@ jest.mock('../../../common/RichTextEditor/RichTextEditor', () => {
4547
});
4648

4749
jest.mock('./ParameterForm', () => {
48-
return jest.fn().mockImplementation(() => <div>ParameterForm.component</div>);
50+
const { Form } = require('antd');
51+
52+
function ParameterFormMock({
53+
definition,
54+
}: Readonly<{
55+
definition?: {
56+
parameterDefinition?: Array<{ name: string; dataType: string }>;
57+
};
58+
}>) {
59+
let params: Record<string, unknown> = {};
60+
61+
try {
62+
const form = Form.useFormInstance();
63+
params = form.getFieldValue('params') ?? {};
64+
} catch {
65+
// no form context
66+
}
67+
68+
const booleanParams = (definition?.parameterDefinition ?? []).filter(
69+
(param) => param.dataType === 'BOOLEAN'
70+
);
71+
72+
return (
73+
<div>
74+
ParameterForm.component
75+
{booleanParams.map((param) => (
76+
<button
77+
aria-checked={params[param.name] ? 'true' : 'false'}
78+
key={param.name}
79+
role="switch"
80+
/>
81+
))}
82+
</div>
83+
);
84+
}
85+
86+
return jest.fn().mockImplementation(ParameterFormMock);
4987
});
5088

5189
jest.mock('../../../../pages/TasksPage/shared/TagSuggestion', () =>
@@ -687,6 +725,82 @@ describe('EditTestCaseModalV1 Component', () => {
687725
});
688726

689727
// Cleanup
690-
document.body.removeChild(testElement);
728+
testElement.remove();
729+
});
730+
731+
it('should correctly convert boolean parameter values from string to boolean', async () => {
732+
(getTestDefinitionById as jest.Mock).mockResolvedValue(
733+
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_BE_IN_SET
734+
);
735+
736+
const testCaseWithBooleanFalse = {
737+
...MOCK_TEST_CASE_WITH_BOOLEAN_PARAM,
738+
parameterValues: [
739+
{
740+
name: 'allowedValues',
741+
value: '["active","inactive","pending"]',
742+
},
743+
{
744+
name: 'matchEnum',
745+
value: 'false',
746+
},
747+
],
748+
};
749+
750+
render(
751+
<EditTestCaseModalV1 {...mockProps} testCase={testCaseWithBooleanFalse} />
752+
);
753+
754+
await waitFor(() => {
755+
expect(screen.getByTestId('edit-test-form')).toBeInTheDocument();
756+
});
757+
758+
const switchElement = await waitFor(() => {
759+
const el = document.querySelector('button[role="switch"]');
760+
761+
expect(el).toBeInTheDocument();
762+
763+
return el as HTMLButtonElement;
764+
});
765+
766+
expect(switchElement.getAttribute('aria-checked')).toBe('false');
767+
});
768+
769+
it('should correctly convert boolean parameter values when value is "true"', async () => {
770+
(getTestDefinitionById as jest.Mock).mockResolvedValue(
771+
MOCK_TEST_DEFINITION_COLUMN_VALUES_TO_BE_IN_SET
772+
);
773+
774+
const testCaseWithBooleanTrue = {
775+
...MOCK_TEST_CASE_WITH_BOOLEAN_PARAM,
776+
parameterValues: [
777+
{
778+
name: 'allowedValues',
779+
value: '["active","inactive","pending"]',
780+
},
781+
{
782+
name: 'matchEnum',
783+
value: 'true',
784+
},
785+
],
786+
};
787+
788+
render(
789+
<EditTestCaseModalV1 {...mockProps} testCase={testCaseWithBooleanTrue} />
790+
);
791+
792+
await waitFor(() => {
793+
expect(screen.getByTestId('edit-test-form')).toBeInTheDocument();
794+
});
795+
796+
const switchElement = await waitFor(() => {
797+
const el = document.querySelector('button[role="switch"]');
798+
799+
expect(el).toBeInTheDocument();
800+
801+
return el as HTMLButtonElement;
802+
});
803+
804+
expect(switchElement.getAttribute('aria-checked')).toBe('true');
691805
});
692806
});

‎openmetadata-ui/src/main/resources/ui/src/components/DataQuality/AddDataQualityTest/components/EditTestCaseModalV1.tsx‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,13 @@ const EditTestCaseModalV1: FC<EditTestCaseModalProps> = ({
357357
};
358358
}
359359

360+
if (param?.dataType === TestDataType.Boolean) {
361+
return {
362+
...acc,
363+
[curr.name || '']: curr.value === 'true',
364+
};
365+
}
366+
360367
return {
361368
...acc,
362369
[curr.name || '']: curr.value,

‎openmetadata-ui/src/main/resources/ui/src/components/DataQuality/AddDataQualityTest/components/TestCaseForm.test.tsx‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
TagSource,
2626
} from '../../../../generated/type/tagLabel';
2727
import { MOCK_TABLE } from '../../../../mocks/TableData.mock';
28+
import { MOCK_TEST_CASE_WITH_BOOLEAN_PARAM } from '../../../../mocks/TestSuite.mock';
2829
import { getListTestDefinitions } from '../../../../rest/testAPI';
2930
import TestCaseForm from './TestCaseForm';
3031

@@ -492,4 +493,46 @@ describe('TestCaseForm', () => {
492493
// Verify that the tags field has the expected data-testid
493494
expect(tagsField).toHaveAttribute('data-testid', 'tags-selector');
494495
});
496+
497+
it('should not crash when array parameter has invalid JSON value', async () => {
498+
const invalidJsonTestCase = {
499+
name: MOCK_TEST_CASE_WITH_BOOLEAN_PARAM.name,
500+
entityLink: MOCK_TEST_CASE_WITH_BOOLEAN_PARAM.entityLink,
501+
testDefinition:
502+
MOCK_TEST_CASE_WITH_BOOLEAN_PARAM.testDefinition.fullyQualifiedName,
503+
parameterValues: [
504+
{ name: 'allowedValues', value: 'not-valid-json' },
505+
{ name: 'matchEnum', value: 'false' },
506+
],
507+
};
508+
509+
await act(async () => {
510+
render(
511+
<TestCaseForm {...mockProps} initialValue={invalidJsonTestCase} />
512+
);
513+
});
514+
515+
// Form renders without crashing despite invalid JSON
516+
expect(await screen.findByTestId('test-case-form')).toBeInTheDocument();
517+
});
518+
519+
it('should not crash when array parameter has valid JSON array value', async () => {
520+
const validArrayTestCase = {
521+
name: MOCK_TEST_CASE_WITH_BOOLEAN_PARAM.name,
522+
entityLink: MOCK_TEST_CASE_WITH_BOOLEAN_PARAM.entityLink,
523+
testDefinition:
524+
MOCK_TEST_CASE_WITH_BOOLEAN_PARAM.testDefinition.fullyQualifiedName,
525+
parameterValues: [
526+
{ name: 'allowedValues', value: '["active","inactive","pending"]' },
527+
{ name: 'matchEnum', value: 'false' },
528+
],
529+
};
530+
531+
await act(async () => {
532+
render(<TestCaseForm {...mockProps} initialValue={validArrayTestCase} />);
533+
});
534+
535+
// Form renders without crashing despite valid JSON array parameters
536+
expect(await screen.findByTestId('test-case-form')).toBeInTheDocument();
537+
});
495538
});

0 commit comments

Comments
 (0)