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

[max-expects] now considers expect.assertions(n) an expect call #1193

Closed
short-dsb opened this issue Aug 8, 2022 · 5 comments · Fixed by #1194
Closed

[max-expects] now considers expect.assertions(n) an expect call #1193

short-dsb opened this issue Aug 8, 2022 · 5 comments · Fixed by #1194

Comments

@short-dsb
Copy link

The behavior in 26.8.1 changed from 26.7.0 to include expect.assertions when counting expect calls. This seems at odds with the purpose of expect.assertions to verify the number of intended expect calls. In 26.8.1, this means either dropping the verification or dropping a different expect, neither of which seem ideal.

My suggestions (in order of personal preference):

  • Restore the behavior present in 26.7.0 and ignore expect.assertions calls when counting max-expects.
  • Provide an option to restore the previous behavior, e.g., ignoreExpectAssertions: boolean.
  • Update the documentation and tests to demonstrate the new behavior.

Thanks for your time and consideration. 🙂

@G-Rath G-Rath added the bug label Aug 9, 2022
@G-Rath
Copy link
Collaborator

G-Rath commented Aug 9, 2022

Yup that's the same sort of thing as #1186 - we shouldn't count expect.<member>() as assertion calls.

@G-Rath G-Rath changed the title jest/max-expects now considers expect.assertions(n) an expect call [max-expects] now considers expect.assertions(n) an expect call Aug 9, 2022
@sir-gon
Copy link

sir-gon commented Aug 9, 2022

This is a bit annoying...

All my 5-assertion tests are broken now, because is counting 6 (???).

This is my example explicitly says 5 assertions:

https://github.com/sir-gon/projecteuler-js/blob/main/src/helpers/bigNumbers.test.js#L44-L62


  it('bigSumMany examples', () => {
    expect.assertions(5);

    expect(bigSumMany(['2', '2'])).toBe('4');
    expect(bigSumMany(['123', '321'])).toBe('444');

    expect(bigSumMany(['2', '2', '2'])).toBe('6');

    expect(bigSumMany(['349', '854', '213', '543'])).toBe('1959');

    expect(
      bigSumMany([
        '12345678901234567890',
        '12345678901234567890',
        '12345678901234567890',
        '12345678901234567890'
      ])
    ).toBe('49382715604938271560');
  });

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 9, 2022

@sir-gon PRs are always welcome; otherwise I'll try and get this resolved myself tonight once I'm off work 🙂

@github-actions
Copy link

github-actions bot commented Aug 9, 2022

🎉 This issue has been resolved in version 26.8.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@short-dsb
Copy link
Author

Thanks for the quick fix, @G-Rath! Your work is appreciated. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants