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

Remove dependency on jest-each #611

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

ericcornelissen
Copy link
Contributor

@ericcornelissen ericcornelissen commented Jul 11, 2023

Description:
Change the dependency on jest-each from a regular dependency to a development dependency (devDependency) as it is only used in tests. Remove the dependency on jest-each in favor of Jest's builtin test.each functionality.

Related issue:
This relates to #394 which introduced jest-each as a dependency.

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

Change the dependency on `jest-each` from a regular dependency to a
development dependency (devDependency) as it is only used in tests.
@ericcornelissen ericcornelissen requested a review from a team as a code owner July 11, 2023 06:02
@ericcornelissen
Copy link
Contributor Author

ericcornelissen commented Jul 11, 2023

As a second suggestion, the functionality of jest-each has been part of the Jest core since v23 (ref). For the version of Jest used in this project (as well as later versions) it is available as test.each.

I did not see the decision to use the package jest-each over the built-in test.each function being discussed in #394. I'd propose to make that switch, if you agree I can update this Pull Request accordingly.

As a reference, the diff would look something like (in addition to removing the dependency on jest-each altogether):

  import path from 'path';
  import fs from 'fs';
- import each from 'jest-each';

  jest.mock('@actions/core');

...

    });

-   each([
+   test.each([
      [new HttpError('Error message')],
      [new NotFound('Error message')]
-   ]).test(
+   ])(
      'should warn if configuration file could not be fetched through the API, log error and fail the action',
      async error => {

@IvanZosimov
Copy link
Contributor

Hi, @ericcornelissen 👋 Thank you for noticing this! For sure, please, switch the code to use the built-in test.each. Moreover, in that case, jest-each can be removed from the list of dependencies completely.

@IvanZosimov IvanZosimov self-assigned this Jul 11, 2023
@ericcornelissen
Copy link
Contributor Author

For sure, please, switch the code to use the built-in test.each. Moreover, in that case, jest-each can be removed from the list of dependencies completely.

Done and done 🙂

@MaksimZhukov MaksimZhukov merged commit b3ae1bd into actions:main Jul 11, 2023
7 checks passed
@ericcornelissen ericcornelissen changed the title Make jest-each a devDependency Remove dependency on jest-each Jul 11, 2023
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.

None yet

4 participants