Skip to content

fix: add onTestFinished to testContext#494

Merged
9aoy merged 3 commits intomainfrom
cleanup-child-process
Aug 21, 2025
Merged

fix: add onTestFinished to testContext#494
9aoy merged 3 commits intomainfrom
cleanup-child-process

Conversation

@9aoy
Copy link
Copy Markdown
Collaborator

@9aoy 9aoy commented Aug 21, 2025 •
edited
Loading

Summary

  • fix: add onTestFinished to testContext. should use onTestFinished hook from the test context when you are running tests concurrently.
describe.concurrent('concurrent suite', () => {
  test('test 1', async ({ onTestFinished }) => {
    /* ... */
  });
  test('test 2', async ({ onTestFinished }) => {
    /* ... */
  });
});
  • update test cases: cleanup child process when test finished. fix some child processes not exit when the test failed in rstest's e2e tests.
image

Related Links

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copilot AI review requested due to automatic review settings August 21, 2025 07:53
@netlify
Copy link
Copy Markdown

netlify bot commented Aug 21, 2025 •
edited
Loading

✅ Deploy Preview for rstest-dev ready!

Name Link
🔨 Latest commit a2e5d28
🔍 Latest deploy log https://app.netlify.com/projects/rstest-dev/deploys/68a6df7a213e680008c584e1
😎 Deploy Preview https://deploy-preview-494--rstest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot AI reviewed Aug 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a resource cleanup issue in e2e tests where child processes were not being properly terminated when tests failed, leading to lingering processes. The change ensures proper cleanup by adding a test lifecycle hook to kill child processes when tests finish.

Key Changes

  • Adds automatic child process cleanup using rstest's onTestFinished hook
  • Prevents orphaned processes when e2e tests fail or complete

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

e2e/scripts/index.ts
const cli = new Cli(process);

onTestFinished(() => {
cli.exec.kill();
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

The code assumes cli.exec exists and has a kill() method, but there's no null check. If cli.exec is undefined or the process hasn't started yet, this will throw a runtime error. Consider adding a check like cli.exec?.kill(); or if (cli.exec) cli.exec.kill();

Suggested change
cli.exec.kill();
cli.exec?.kill();

Copilot uses AI. Check for mistakes.
e2e/scripts/index.ts
onTestFinished(() => {
cli.exec.kill();
});

Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

The onTestFinished callback is registered after cli.exec is created, but if the test fails immediately or the process exits quickly, there could be a race condition where the cleanup handler isn't registered in time. Consider registering the cleanup handler before starting the process or using a more robust cleanup mechanism.

Suggested change

Copilot uses AI. Check for mistakes.
@9aoy 9aoy changed the title test: cleanup child process when test finished fix: add onTestFinished to testContext Aug 21, 2025
@9aoy 9aoy merged commit 0e65e7c into main Aug 21, 2025
17 checks passed
@9aoy 9aoy deleted the cleanup-child-process branch August 21, 2025 09:07
@9aoy 9aoy mentioned this pull request Aug 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

change: fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@9aoy