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

improve includeSameMembers error message #640

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rluvaton
Copy link
Contributor

@rluvaton rluvaton commented Aug 3, 2023

What

Improve the error message in case actual and expected do not match

Why

#639

Notes

Before I add the matching coverage I would appreciate a review as there may be some resistant...

Housekeeping

  • Unit tests
  • Documentation is up to date
  • No additional lint warnings
  • Typescript definitions are added/updated where relevant

How would it look like:

For the following tests:

Tests
describe('compare', () => {
  test('fails when the arrays are not equal in length', () => {
    expect([1, 2]).toIncludeSameMembers([1]);
  });

  test('fails when not passed array', () => {
    expect(2).toIncludeSameMembers([1]);
  });

  describe('fails when actual has more items than expected (when the ones exists match)', () => {
    test('simple items', () => {
      expect([2, 4, 3, 1]).toIncludeSameMembers([1, 2, 3]);
    });

    test('objects', () => {
      expect([{ id: 2 }, { id: 4 }, { id: 3 }, { id: 1 }]).toIncludeSameMembers([{ id: 1 }, { id: 2 }, { id: 3 }]);
    });
  });

  describe('fails when actual has less items than expected (when the ones exists match)', () => {
    test('simple items', () => {
      expect([2, 3, 1]).toIncludeSameMembers([1, 2, 3, 4]);
    });

    test('objects', () => {
      expect([{ id: 2 }, { id: 3 }, { id: 1 }]).toIncludeSameMembers([{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }]);
    });
  });

  describe('fails when actual has more items than expected (when the ones exists not all match)', () => {
    test('simple items', () => {
      expect([3, 1, 8, 5, 6]).toIncludeSameMembers([1, 2, 3]);
    });

    test('objects', () => {
      expect([{ id: 3 }, { id: 1 }, { id: 8 }, { id: 5 }, { id: 6 }]).toIncludeSameMembers([
        { id: 1 },
        { id: 2 },
        { id: 3 },
      ]);
    });
  });

  describe('have gaps', () => {
    test('simple items', () => {
      expect([5, 6, 1]).toIncludeSameMembers([1, 2, 3, 4, 5]);
    });

    test('objects', () => {
      expect([
        { e: 5, value: 'thanks, you' },
        { f: 6, value: '?' },
        { a: 1, value: 'no' },
      ]).toIncludeSameMembers([
        { a: 1, value: 'hello' },
        { b: 2, value: 'world' },
        { c: 3, value: 'how are you' },
        { d: 4, value: 'im good' },
        { e: 5, value: 'thanks, you' },
      ]);
    });
  });
});

This is the result:

Current

old.mov

After this PR

new.mov

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2023

🦋 Changeset detected

Latest commit: f50a1bc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
jest-extended Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (711fdcc) 100.00% compared to head (f51f475) 99.21%.

❗ Current head f51f475 differs from pull request most recent head f50a1bc. Consider uploading reports for the commit f50a1bc to get more accurate results

Files Patch % Lines
src/matchers/toIncludeSameMembers.js 93.87% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #640      +/-   ##
===========================================
- Coverage   100.00%   99.21%   -0.79%     
===========================================
  Files           73       73              
  Lines          674      767      +93     
  Branches       290      315      +25     
===========================================
+ Hits           674      761      +87     
- Misses           0        6       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rluvaton rluvaton marked this pull request as ready for review August 3, 2023 20:22
@rluvaton
Copy link
Contributor Author

rluvaton commented Aug 3, 2023

The failures are coverage failures just wanna get some feedback before putting the time fixing them

@rluvaton
Copy link
Contributor Author

rluvaton commented Aug 5, 2023

Cc @SimenB

@rluvaton
Copy link
Contributor Author

Ping 😄

@rluvaton
Copy link
Contributor Author

Cc @keeganwitt

@SebTVisage
Copy link

Any update on this? The current output is unusable as soon as you have a big array or "big" objects in it

@rluvaton
Copy link
Contributor Author

@SebTVisage Until and if the related PR gets merged I created an npm package with that matcher and the better output:
https://github.com/rluvaton/expect-matchers (I also improved the performance for primitives - rluvaton/expect-matchers#7)

@keeganwitt
Copy link
Collaborator

I'll try to take a look at this.

@rluvaton
Copy link
Contributor Author

rluvaton commented Feb 2, 2024

Thank you

After this is merged I will create a performance improvement for that function for primitive values

@keeganwitt
Copy link
Collaborator

@SimenB any thoughts on this idea?

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