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

Allow test titles to include array index #6414

Merged
merged 9 commits into from
Aug 8, 2018
Merged

Allow test titles to include array index #6414

merged 9 commits into from
Aug 8, 2018

Conversation

mfix22
Copy link

@mfix22 mfix22 commented Jun 7, 2018

Updates jest-each so that titles can include the index of the test case. I am not set by any means on the '%_' placeholder. Let me know if you would like to change the value of if this is already currently possible. I will update the README once this has been reviewed.

Summary

In the past I have always take the index from [].forEach((value, i) => {}) and passed it into the title for parameterized tests. This would make it simpler by integrating jest-each.

Test plan

Tests have been added 👍

expectFunction,
);
expect(globalMock).toHaveBeenCalledWith(
`expected string: world 1 null undefined 1.2 ${JSON.stringify({
baz: 'qux',
})} () => {} [] Infinity NaN`,
})} () => {} [] Infinity NaN 0`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be 1 as it’s the second row of test data

Copy link
Author

@mfix22 mfix22 Jun 8, 2018

Choose a reason for hiding this comment

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

🤦‍♂️ of course yeah! 😄

),
title: acc.title
.replace(PRETTY_PLACEHOLDER, pretty(arg, {maxDepth: 1, min: true}))
.replace(INDEX_PLACEHOLDER, `${index}`),
Copy link
Contributor

Choose a reason for hiding this comment

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

This index represents the index of each arg in a row of data.

I think you want to use the index of each row in the outer table on line 32.

return table.forEach(row =>
  cb(arrayFormat(title, ...row), applyRestParams(row, test)),
);

@mfix22
Copy link
Author

mfix22 commented Jun 8, 2018

@mattphillips thanks for the review 😃 ! Probably should have given that another look before pushing 🤷‍♂️ . I also made the place holder %# for the index. What do you think?

@mfix22
Copy link
Author

mfix22 commented Jun 11, 2018

@mattphillips anything else I need to update here? 😃

),
title: acc.title
.replace(PRETTY_PLACEHOLDER, pretty(arg, {maxDepth: 1, min: true}))
.replace(INDEX_PLACEHOLDER, `${rowIndex}`),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this and line 97 can move outside of the reduce and just do a replace on the prettyTitle returned before calling util.format on line 103.

Maybe something like:

return util.format(
    prettyTitle.replace(INDEX_PLACEHOLDER, rowIndex.toString()),
    ...remainingArgs.slice(0, placeholders.length - prettyIndexes.length),
  );

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mfix22 anything preventing us from applying this solution? Seems more generic approach

Copy link
Author

Choose a reason for hiding this comment

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

Oooh i missed this. I'll take a look.

@mattphillips
Copy link
Contributor

I think I prefer %# over %_ for the index 😄

@SimenB @thymikee @rickhanlonii do you guys have any opinion with this?

If not @mfix22 can you update the docs with %# as an option see:

It might also be worth adding an integration test for this in the e2e directory

@rickhanlonii
Copy link
Member

Between the two I would prefer %# since _ already has a few meanings (like "private" and ignored values)

@codecov-io
Copy link

codecov-io commented Jun 11, 2018

Codecov Report

Merging #6414 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6414      +/-   ##
==========================================
+ Coverage   63.62%   63.63%   +<.01%     
==========================================
  Files         235      235              
  Lines        9018     9019       +1     
  Branches        4        3       -1     
==========================================
+ Hits         5738     5739       +1     
  Misses       3279     3279              
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-each/src/bind.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f66b74d...0650427. Read the comment docs.

@mfix22
Copy link
Author

mfix22 commented Jul 20, 2018

@mattphillips @rickhanlonii anything I need to do to get this PR merged?

@SimenB
Copy link
Member

SimenB commented Jul 21, 2018

I completely missed this, sorry! Could you add an entry to the changelog?

@mfix22
Copy link
Author

mfix22 commented Jul 22, 2018

@SimenB no worries! Let me know if there is anything else.

@mfix22
Copy link
Author

mfix22 commented Jul 22, 2018

@thymikee @mattphillips thanks for catching that! That's a much better approach.

@thymikee thymikee merged commit b4b1eee into jestjs:master Aug 8, 2018
@mfix22
Copy link
Author

mfix22 commented Aug 8, 2018

Woo!!! Thanks everyone!

@mfix22 mfix22 deleted the jest-each-index branch August 8, 2018 20:10
thymikee added a commit to rhysawilliams2010/jest that referenced this pull request Aug 8, 2018
* upstream/master: (122 commits)
  fix: don't report promises as open handles (jestjs#6716)
  support serializing `DocumentFragment` (jestjs#6705)
  Allow test titles to include array index (jestjs#6414)
  fix `toContain` suggest to contain equal message (jestjs#6810)
  fix: testMatch not working with negations (jestjs#6648)
  Add test cases for jestjs#6744 (jestjs#6772)
  print stack trace on calls to process.exit (jestjs#6714)
  Updates SnapshotTesting.md to provide more info. on snapshot scope (jestjs#6735)
  Mark snapshots as obsolete when moved to an inline snapshot (jestjs#6773)
  [Docs] Clarified the use of literal values as property matchers in toMatchSnapshot() (jestjs#6807)
  Update CHANGELOG.md (jestjs#6799)
  fix changelog entry that is not in 23.4.2 (jestjs#6796)
  Fix --coverage with --findRelatedTests overwriting collectCoverageFrom options (jestjs#6736)
  Update testURL default value from about:blank to localhost (jestjs#6792)
  fix: `matchInlineSnapshot` when prettier dependencies are mocked (jestjs#6776)
  Fix retryTimes and add e2e regression test (jestjs#6762)
  Release v23.4.2
  Docs/ExpectAPI: Correct docs for `objectContaining` (jestjs#6754)
  chore(packages/babel-jest) readme (jestjs#6746)
  docs: noted --coverage aliased by --collectCoverage (jestjs#6741)
  ...
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants