Skip to content

Fix bugs with setup and teardown#68

Merged
DmitrySharabin merged 5 commits intomainfrom
setup-teardown-bugs
Jan 29, 2025
Merged

Fix bugs with setup and teardown#68
DmitrySharabin merged 5 commits intomainfrom
setup-teardown-bugs

Conversation

@DmitrySharabin
Copy link
Copy Markdown
Member

  • Maintain proper sequencing when hooks (beforeEach/afterEach, beforeAll/afterAll) are used
  • Fall back to parallel execution (using Promise.all) when no hooks exist

Now, we might end up (I already was bit by this bug) calling afterAll before afterEach for some tests. In most cases, beforeEach for all tests is called before the first test runs. There are probably some more issues we haven't encountered yet. This PR should fix those bugs.

@netlify
Copy link
Copy Markdown

netlify bot commented Jan 25, 2025

Deploy Preview for h-test ready!

Name Link
🔨 Latest commit 64de46f
🔍 Latest deploy log https://app.netlify.com/sites/h-test/deploys/679a538451c87d00088d02f9
😎 Deploy Preview https://deploy-preview-68--h-test.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 site configuration.

- Maintain proper sequencing when hooks (`beforeEach`/`afterEach`, `beforeAll`/`afterAll`) are used
- Fall back to parallel execution (using `Promise.all`) when no hooks exist
Copy link
Copy Markdown
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

The way I see it, we can still do parallel execution with these, as long as:

  • For each test, beforeEach is always executed before run and afterEach after run
  • beforeAll and afterAll are executed before and after all tests (regardless of their order)

@DmitrySharabin
Copy link
Copy Markdown
Member Author

The way I see it, we can still do parallel execution with these, as long as:

  • For each test, beforeEach is always executed before run and afterEach after run
  • beforeAll and afterAll are executed before and after all tests (regardless of their order)

You are absolutely right. Previously, I was confused seeing beforeEach of a test before run of the previous one. But now I understand it's OK if the order of hooks for individual tests is preserved. It should be fixed now.

Comment thread src/classes/TestResult.js Outdated
Copy link
Copy Markdown
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

This looks a bit too complicated and I have some reservations about whether the complexity is necessary, but I may be missing some of the nuances of this part of the code.

  • allFinished is added dynamically, which means it could not be present when awaited from another part of the code
  • if runAll() resolves when all tests have ran, why do we need allFinished? Can't we just Promise.allSettled() its result?
  • Remember that in theory, tests can be run in isolation, without running the whole group. At first glance it seems to me that this wouldn't run beforeAll/afterAll, but I didn't look too closely.

@DmitrySharabin
Copy link
Copy Markdown
Member Author

This looks a bit too complicated and I have some reservations about whether the complexity is necessary, but I may be missing some of the nuances of this part of the code.

  • allFinished is added dynamically, which means it could not be present when awaited from another part of the code
  • if runAll() resolves when all tests have ran, why do we need allFinished? Can't we just Promise.allSettled() its result?
  • Remember that in theory, tests can be run in isolation, without running the whole group. At first glance it seems to me that this wouldn't run beforeAll/afterAll, but I didn't look too closely.

What an excellent feedback (as always)! Thank you.

I tried to address it:

  • moved beforeEach/afterEach to TestResult.run, so if a test is skipped, those hooks are not called
  • simplified the code (got rid of redundant allFinished in favor of Promise.allSettled)
  • beforeAll/afterAll should now run when a test is running in isolation

I'll be grateful if you could find some time to review this PR one more time.

@DmitrySharabin DmitrySharabin merged commit 9edf938 into main Jan 29, 2025
@DmitrySharabin DmitrySharabin deleted the setup-teardown-bugs branch January 29, 2025 22:29
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