-
Notifications
You must be signed in to change notification settings - Fork 16.7k
fix(test): changed test use unsaved changes prompt #35447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
19151d1
35aaeb6
db3afda
9eea2e6
3930c95
b355212
7200d6c
3049a3d
247aef8
b4678ec
0ae844d
49fc529
601dde2
66823a5
4f59629
a7ce4e9
1bf4fdd
1241867
045cf6b
acdba8a
4fb89c2
f47d28b
3bdf337
780a5d6
3c339df
798b7a6
2db564e
a58cfad
9ea0d2f
c6bbf53
5214e7a
d98105c
30b1576
4aaa27c
25c1987
c3854cb
e1c4909
b1897dd
f3e973f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,115 +22,116 @@ import { Router } from 'react-router-dom'; | |
| import { createMemoryHistory } from 'history'; | ||
| import { act } from 'spec/helpers/testing-library'; | ||
|
|
||
| const history = createMemoryHistory({ | ||
| let history = createMemoryHistory({ | ||
| initialEntries: ['/dashboard'], | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| history = createMemoryHistory({ initialEntries: ['/dashboard'] }); | ||
| }); | ||
|
|
||
| const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
| <Router history={history}>{children}</Router> | ||
| ); | ||
|
|
||
| // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks | ||
| describe('useUnsavedChangesPrompt', () => { | ||
| test('should not show modal initially', () => { | ||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave: jest.fn(), | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| expect(result.current.showModal).toBe(false); | ||
| }); | ||
| test('should not show modal initially', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test isolation failure
The PR removes the describe block wrapper and converts tests to standalone test functions, but this creates a critical test isolation issue. All tests now share the same Code suggestionCheck the AI-generated fix before applying Code Review Run #c65c3d Should Bito avoid suggestions like this for future reviews? (Manage Rules)
|
||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave: jest.fn(), | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| expect(result.current.showModal).toBe(false); | ||
| }); | ||
|
|
||
| test('should block navigation and show modal if there are unsaved changes', () => { | ||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave: jest.fn(), | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| // Simulate blocked navigation | ||
| act(() => { | ||
| const unblock = history.block((tx: any) => tx); | ||
| unblock(); | ||
| history.push('/another-page'); | ||
| }); | ||
|
|
||
| expect(result.current.showModal).toBe(true); | ||
| test('should block navigation and show modal if there are unsaved changes', () => { | ||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave: jest.fn(), | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| // Simulate blocked navigation | ||
| act(() => { | ||
| const unblock = history.block((tx: any) => tx); | ||
| unblock(); | ||
| history.push('/another-page'); | ||
| }); | ||
|
|
||
| test('should trigger onSave and hide modal on handleSaveAndCloseModal', async () => { | ||
| const onSave = jest.fn().mockResolvedValue(undefined); | ||
| expect(result.current.showModal).toBe(true); | ||
| }); | ||
|
|
||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave, | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
| test('should trigger onSave and hide modal on handleSaveAndCloseModal', async () => { | ||
| const onSave = jest.fn().mockResolvedValue(undefined); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSaveAndCloseModal(); | ||
| }); | ||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave, | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| expect(onSave).toHaveBeenCalled(); | ||
| expect(result.current.showModal).toBe(false); | ||
| await act(async () => { | ||
| await result.current.handleSaveAndCloseModal(); | ||
| }); | ||
|
|
||
| test('should trigger manual save and not show modal again', async () => { | ||
| const onSave = jest.fn().mockResolvedValue(undefined); | ||
| expect(onSave).toHaveBeenCalled(); | ||
| expect(result.current.showModal).toBe(false); | ||
| }); | ||
|
|
||
| test('should trigger manual save and not show modal again', async () => { | ||
| const onSave = jest.fn().mockResolvedValue(undefined); | ||
|
|
||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave, | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave, | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
| act(() => { | ||
| result.current.triggerManualSave(); | ||
| }); | ||
|
|
||
| act(() => { | ||
| result.current.triggerManualSave(); | ||
| }); | ||
| expect(onSave).toHaveBeenCalled(); | ||
| expect(result.current.showModal).toBe(false); | ||
| }); | ||
|
|
||
| expect(onSave).toHaveBeenCalled(); | ||
| expect(result.current.showModal).toBe(false); | ||
| test('should close modal when handleConfirmNavigation is called', () => { | ||
| const onSave = jest.fn(); | ||
|
|
||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave, | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| // First, trigger navigation to show the modal | ||
| act(() => { | ||
| const unblock = history.block((tx: any) => tx); | ||
| unblock(); | ||
| history.push('/another-page'); | ||
| }); | ||
|
|
||
| it('should close modal when handleConfirmNavigation is called', () => { | ||
| const onSave = jest.fn(); | ||
|
|
||
| const { result } = renderHook( | ||
| () => | ||
| useUnsavedChangesPrompt({ | ||
| hasUnsavedChanges: true, | ||
| onSave, | ||
| }), | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| // First, trigger navigation to show the modal | ||
| act(() => { | ||
| const unblock = history.block((tx: any) => tx); | ||
| unblock(); | ||
| history.push('/another-page'); | ||
| }); | ||
|
|
||
| expect(result.current.showModal).toBe(true); | ||
|
|
||
| // Then call handleConfirmNavigation to discard changes | ||
| act(() => { | ||
| result.current.handleConfirmNavigation(); | ||
| }); | ||
|
|
||
| expect(result.current.showModal).toBe(false); | ||
| expect(result.current.showModal).toBe(true); | ||
|
|
||
| // Then call handleConfirmNavigation to discard changes | ||
| act(() => { | ||
| result.current.handleConfirmNavigation(); | ||
| }); | ||
|
|
||
| expect(result.current.showModal).toBe(false); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beforeEach should work on a flat structure as well right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, beforeEach works perfectly fine with a flat test structure.
It runs before each test - every test gets a fresh history instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but i don't quite understand this change still? If const was working before do we need this let and reassign in beforeEach?