Allow test titles to include array index #6414
Conversation
| expectFunction, | ||
| ); | ||
| expect(globalMock).toHaveBeenCalledWith( | ||
| `expected string: world 1 null undefined 1.2 ${JSON.stringify({ | ||
| baz: 'qux', | ||
| })} () => {} [] Infinity NaN`, | ||
| })} () => {} [] Infinity NaN 0`, |
There was a problem hiding this comment.
I think this should be 1 as it’s the second row of test data
packages/jest-each/src/bind.js
Outdated
| ), | ||
| title: acc.title | ||
| .replace(PRETTY_PLACEHOLDER, pretty(arg, {maxDepth: 1, min: true})) | ||
| .replace(INDEX_PLACEHOLDER, `${index}`), |
There was a problem hiding this comment.
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)),
);
|
@mattphillips thanks for the review 😃 ! Probably should have given that another look before pushing 🤷♂️ . I also made the place holder |
|
@mattphillips anything else I need to update here? 😃 |
packages/jest-each/src/bind.js
Outdated
| ), | ||
| title: acc.title | ||
| .replace(PRETTY_PLACEHOLDER, pretty(arg, {maxDepth: 1, min: true})) | ||
| .replace(INDEX_PLACEHOLDER, `${rowIndex}`), |
There was a problem hiding this comment.
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),
);
There was a problem hiding this comment.
@mfix22 anything preventing us from applying this solution? Seems more generic approach
There was a problem hiding this comment.
Oooh i missed this. I'll take a look.
|
I think I prefer @SimenB @thymikee @rickhanlonii do you guys have any opinion with this? If not @mfix22 can you update the docs with
It might also be worth adding an integration test for this in the e2e directory |
|
Between the two I would prefer |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
@mattphillips @rickhanlonii anything I need to do to get this PR merged? |
|
I completely missed this, sorry! Could you add an entry to the changelog? |
|
@SimenB no worries! Let me know if there is anything else. |
|
@thymikee @mattphillips thanks for catching that! That's a much better approach. |
|
Woo!!! Thanks everyone! |
* 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) ...
|
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. |
Updates
jest-eachso 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 integratingjest-each.Test plan
Tests have been added 👍