Skip to content

test(compat/each): replace each duplicate tests with an alias check against forEach#1873

Merged
raon0211 merged 1 commit into
toss:mainfrom
Antoliny0919:remove-alias-test-case
Jul 11, 2026
Merged

test(compat/each): replace each duplicate tests with an alias check against forEach#1873
raon0211 merged 1 commit into
toss:mainfrom
Antoliny0919:remove-alias-test-case

Conversation

@Antoliny0919

Copy link
Copy Markdown
Contributor

Summary

We could delegate the specific test cases for the alias each to forEach. Since forEach is already thoroughly tested and each is simply an alias of it, that should be sufficient.

@vercel

vercel Bot commented Jul 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
es-toolkit Ready Ready Preview, Comment Jul 10, 2026 7:29am

Request Review

Comment on lines +6 to +7
it('should be an alias of forEach', () => {
expect(each).toBe(forEach);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I noticed that alias functions didn't have dedicated test files before. How about keeping the test file, as it is now, but only adding a test that verifies the alias references the same implementation as the original function?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As far as I understand, that's actually already what this PR does.

each.spec.ts stays right where it is; we just trimmed it down to a single test confirming each is the same implementation as forEach:

describe('each', () => {
  it('should be an alias of forEach', () => {
    expect(each).toBe(forEach);
  });
});

Only the duplicated behavioral cases were removed, since forEach.spec.ts already covers those. Let me know if you meant something a bit different, happy to adjust!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as I understand, that's actually already what this PR does.

each.spec.ts stays right where it is; we just trimmed it down to a single test confirming each is the same implementation as forEach:

describe('each', () => {
  it('should be an alias of forEach', () => {
    expect(each).toBe(forEach);
  });
});

Only the duplicated behavioral cases were removed, since forEach.spec.ts already covers those. Let me know if you meant something a bit different, happy to adjust!

I actually meant asking whether we should add the same kind of test for the other aliases too, like first, extend ...
It looks like this is already being worked on :) -> #1883

@raon0211 raon0211 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@raon0211 raon0211 merged commit 171d8a8 into toss:main Jul 11, 2026
12 checks passed
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