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

tests: refactor install.test.ts to use jest.each #516

Conversation

aneeshrelan
Copy link
Contributor


name: ⚙ Improvement
about: Refactoring tests to reduce redundancies using jest.each
labels: enhancement


A Pull Request should be associated with a Discussion.

Refactoring the install.test.ts to be using parameters with jest.each

Related discussion: #506

Further notes in Contribution Guidelines
Thank you for your contribution.

Description

This PR improves the test code by refactoring the install.test.ts unit tests using parameterised testing by jest.each

In case this PR introduced TypeScript/JavaScript code changes:

  • I have written test cases for the changes in this pull request
  • I have run npm run format before the commit.
  • I have run npm run lint before the commit.
  • I have run npm run release before the commit.
  • npm test returns with no unit test errors and all code covered.

In case this PR edits any scripts:

  • I have checked the edited scripts for syntax.
  • I have tested the changes in an integration test (If yes, provide workflow YAML and link). - NA

@aneeshrelan
Copy link
Contributor Author

Seems like the __dirname is not matching on a windows environment which is failing the check. I preferred using a Regexp based matching to avoid duplications. Thoughts? @shivammathur

Copy link
Owner

@shivammathur shivammathur left a comment

Choose a reason for hiding this comment

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

Make the OS also an input parameter in the test data, then the 3 test functions can be merged into 1.

@shivammathur
Copy link
Owner

shivammathur commented Oct 9, 2021

@aneeshrelan
When you convert string with path into a regular expression, it breaks on Windows as escaped slashed in path are removed.

Modify the getScript mocked function like below.

let script = 'initial script';
script += tools_csv ? ' add_tool' : '';
script += extension_csv ? ' install extensions' : '';
script += coverage_driver ? ' set coverage driver' : '';
script += ini_values_csv ? ' edit php.ini' : '';
return script;

then you can use exact matching instead of regular expresssions, for example

${'7.3'} | ${'a, b'}     | ${'a=b'}       | ${'x'}          | ${''} | ${`initial script install extensions set coverage driver edit php.ini pwsh win32.ps1 7.3 ${__dirname}`}
expect(await install.run()).toBe(output);

@aneeshrelan
Copy link
Contributor Author

Thanks for the inputs @shivammathur. Resolved feedback, please check. Thanks

Copy link
Owner

@shivammathur shivammathur left a comment

Choose a reason for hiding this comment

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

Looks great

@shivammathur shivammathur merged commit 0cd4af8 into shivammathur:develop Oct 9, 2021
@shivammathur
Copy link
Owner

Merged 👍

@shivammathur shivammathur added awaiting-release Added/Fixed and tested, awaiting release hacktoberfest-accepted labels Oct 9, 2021
@shivammathur shivammathur linked an issue Oct 9, 2021 that may be closed by this pull request
@aneeshrelan aneeshrelan deleted the feature/refactor-install-tests-to-use-jest-each branch October 24, 2021 13:14
@shivammathur shivammathur removed the awaiting-release Added/Fixed and tested, awaiting release label Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor: Use parameterised testing using jest.it.each
2 participants