Skip to content
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

Remove test timers related to code block #380

Merged
merged 2 commits into from
Jul 24, 2024
Merged

Conversation

sejas
Copy link
Member

@sejas sejas commented Jul 23, 2024

Proposed Changes

  • This PR fixes some timers running detected by running npm run test -- --detectOpenHandles. Do not fixes all the cases though.

I've observed that running npm run test throws this warning:

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.

This warning already existed in a704e04 before merging #308

After debugging with npm run test -- --detectOpenHandles, I got some warning that I've addressed in this PR.

Screenshot 2024-07-23 at 17 35 53 Screenshot 2024-07-23 at 17 35 46 Screenshot 2024-07-23 at 17 35 15

Testing Instructions

  • Run npm run test -- --detectOpenHandles
  • Observe the tests pass
  • Observe there is are no warnings pointing to specific files.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@sejas sejas self-assigned this Jul 23, 2024
@sejas sejas requested a review from a team July 23, 2024 18:09
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎊 ! Good catch @sejas !

@@ -100,10 +100,9 @@ describe( 'createCodeComponent', () => {

expect( screen.getByText( 'Running...' ) ).toBeVisible();

// Run code execution measurement timer
await act( () => jest.runAllTimersAsync() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could keep this line if we use fake timers.

diff --git forkSrcPrefix/src/components/tests/assistant-code-block.test.tsx forkDstPrefix/src/components/tests/assistant-code-block.test.tsx
index 2cee11e4fbdacf4383f20ee731f97aaedaadd346..3da280e60c199a619857b02d20ce2d58f64bbfd3 100644
--- forkSrcPrefix/src/components/tests/assistant-code-block.test.tsx
+++ forkDstPrefix/src/components/tests/assistant-code-block.test.tsx
@@ -1,4 +1,4 @@
-import { render, screen, fireEvent, waitFor } from '@testing-library/react';
+import { act, render, screen, fireEvent, waitFor } from '@testing-library/react';
 import { getIpcApi } from '../../lib/get-ipc-api';
 import createCodeComponent from '../assistant-code-block';
 
@@ -89,6 +89,14 @@ describe( 'createCodeComponent', () => {
 	} );
 
 	describe( 'when the "run" button is clicked', () => {
+		beforeEach( () => {
+			jest.useFakeTimers();
+		} );
+
+		afterEach( () => {
+			jest.useRealTimers();
+		} );
+
 		it( 'should display an activity indicator while running code', async () => {
 			( getIpcApi as jest.Mock ).mockReturnValue( {
 				executeWPCLiInline: jest.fn().mockResolvedValue( { stdout: 'Mock success', stderr: '' } ),
@@ -100,9 +108,9 @@ describe( 'createCodeComponent', () => {
 
 			expect( screen.getByText( 'Running...' ) ).toBeVisible();
 
-			await waitFor( () => {
-				expect( screen.queryByText( 'Running...' ) ).not.toBeInTheDocument();
-			} );
+			await act( () => jest.runOnlyPendingTimersAsync() );
+
+			expect( screen.queryByText( 'Running...' ) ).not.toBeInTheDocument();
 		} );
 
 		it( 'should display the output of the successfully executed code', async () => {
@@ -113,10 +121,10 @@ describe( 'createCodeComponent', () => {
 
 			fireEvent.click( screen.getByText( 'Run' ) );
 
-			await waitFor( () => {
-				expect( screen.getByText( 'Success' ) ).toBeVisible();
-				expect( screen.getByText( 'Mock success' ) ).toBeVisible();
-			} );
+			await act( () => jest.runOnlyPendingTimersAsync() );
+
+			expect( screen.getByText( 'Success' ) ).toBeVisible();
+			expect( screen.getByText( 'Mock success' ) ).toBeVisible();
 		} );
 
 		it( 'should display the output of the failed code execution', async () => {
@@ -127,10 +135,10 @@ describe( 'createCodeComponent', () => {
 
 			fireEvent.click( screen.getByText( 'Run' ) );
 
-			await waitFor( () => {
-				expect( screen.getByText( 'Error' ) ).toBeVisible();
-				expect( screen.getByText( 'Mock error' ) ).toBeVisible();
-			} );
+			await act( () => jest.runOnlyPendingTimersAsync() );
+
+			expect( screen.getByText( 'Error' ) ).toBeVisible();
+			expect( screen.getByText( 'Mock error' ) ).toBeVisible();
 		} );
 	} );
 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good! I reverted that change in favor of the timers cdb3e17

@sejas sejas merged commit bc68f5e into trunk Jul 24, 2024
10 checks passed
@sejas sejas deleted the update/tests-with-timers branch July 24, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants