bito-code-review[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4170409239
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when
operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending
edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred
value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={undefined} />);
Review Comment:
<!-- Bito Reply -->
The reviewer is correct that passing `undefined` to `clearAllTrigger` does
not explicitly release the trigger if the underlying prop is optional and
omitted by the spread operator. To ensure the trigger is explicitly released
and the test correctly exercises the release behavior, you should pass `{
'test-filter': false }` instead of `undefined`.
**superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx**
```
rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={{
'test-filter': false }} />);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
