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

confirm tests assert when run within an generator function #55

Merged
merged 9 commits into from
Jan 4, 2025

Conversation

jbolda
Copy link
Collaborator

@jbolda jbolda commented Dec 30, 2024

Motivation

We had a few tests that could include a race condition and not actually run any assertions. Add a check to confirm the number of assertions for each test that has this type of pattern. Any test that executes functions and asserts on the return shouldn't require this check.

@jbolda jbolda requested review from neurosnap and VldMrgnn December 30, 2024 19:28
@VldMrgnn VldMrgnn marked this pull request as ready for review January 1, 2025 17:25
Copy link
Collaborator

@VldMrgnn VldMrgnn left a comment

Choose a reason for hiding this comment

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

Hi Jacob,
Great PR on identifying the tests with potential race conditions—sharp catch! I went ahead and fixed a couple of the tests you pointed out that weren’t actually being executed but were reporting success.
Your attention to detail really highlights an issue that might have gone unnoticed otherwise.

@VldMrgnn
Copy link
Collaborator

VldMrgnn commented Jan 1, 2025

Following @jbolda's pattern, I noticed that thunk.test.ts was also missing expect.assertions, resulting in at least two tests incorrectly reporting success without actually running. To address this, I’ve added expect.assertions to all tests to ensure the correct number of assertions are made. Consequently, I replaced asserts.assertEquals with expect assertions for consistency and accuracy.

@neurosnap neurosnap merged commit 1f2e083 into main Jan 4, 2025
4 checks passed
@neurosnap neurosnap deleted the confirm-test-assertions branch January 4, 2025 14:03
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.

3 participants