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

[jest-each]: Add support for keyPaths in test titles #6457

Merged
merged 10 commits into from
Jun 20, 2018

Conversation

mattphillips
Copy link
Contributor

@mattphillips mattphillips commented Jun 13, 2018

Summary

Closes #6452

Opening this up for review early as I want @SimenB @thymikee @rickhanlonii opinion on how to share the getPath logic that is defined in expect.

Add support for $key.path syntax in tagged template test titles.

Example:

// Actions
const increment = () => ({type: 'increment'});
const decrement = () => ({type: 'decrement'});

// Reducer
const reducer = (state = 0, {type}) => {
  switch (type) {
    case 'increment':
      return state + 1;
    case 'decrement':
      return state - 1;
    default:
      return state;
  }
};

test.each`
  state | action         | nextState
  ${0}  | ${increment()} | ${1}
  ${0}  | ${decrement()} | ${-1}
`(
  'returns $nextState when the state is $state and $action.type is dispatched $nextState',
  ({state, action, nextState}) => {
    expect(reducer(state, action)).toEqual(nextState);
  },
);

screen shot 2018-06-13 at 13 32 25

Test plan

See unit tests

TODO:

  • Changelog
  • Update docs
  • Some how share the getPath logic with expect

Object.prototype.hasOwnProperty.call(object, value) ||
Object.prototype.hasOwnProperty.call(object.constructor.prototype, value);

const getPath = (object: Object, propertyPath: string | Array<string>) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've inlined this for now to get it working, it shouldn't stay here :)

Copy link
Member

Choose a reason for hiding this comment

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

can we just use lodash.get or something?

Copy link
Contributor Author

@mattphillips mattphillips Jun 13, 2018

Choose a reason for hiding this comment

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

I didn't think we were using lodash anywhere? This getPath algorithm is used in the toHaveProperty matcher and offers a nicer returned value than lodash as it tracks the last visited object.

EDIT: see expect utils

Copy link
Member

Choose a reason for hiding this comment

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

sticking it in jest-utils should be fine for sharing (although @cpojer hates the util package :D)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I move it as part of this PR or would it be better in a separate PR?

@SimenB
Copy link
Member

SimenB commented Jun 13, 2018

As part of this is fine

@mattphillips
Copy link
Contributor Author

mattphillips commented Jun 13, 2018

@SimenB have you seen an eslint error like this before?

https://circleci.com/gh/facebook/jest/25790?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Not sure what is going wrong 😕

@SimenB
Copy link
Member

SimenB commented Jun 13, 2018

Syntax error in the browserbuild, which must be es5

@mattphillips
Copy link
Contributor Author

Hmm @SimenB so it looks like expect cannot depend on jest-util then? How else can we share the getPath code?

@codecov-io
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #6457 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6457      +/-   ##
==========================================
+ Coverage   63.48%   63.53%   +0.05%     
==========================================
  Files         227      227              
  Lines        8697     8709      +12     
  Branches        4        3       -1     
==========================================
+ Hits         5521     5533      +12     
  Misses       3175     3175              
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-each/src/bind.js 100% <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 9c1c3b1...341997a. Read the comment docs.

@mattphillips
Copy link
Contributor Author

@SimenB I've updated this to inline a much simpler getPath function so no need to add anything to jest-util

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
## master

### Features

- `[jest-each]` Add support for keyPaths in test titles ([#6457](https://github.com/facebook/jest/pull/6457/files))
Copy link
Member

Choose a reason for hiding this comment

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

remove /files

@cpojer cpojer merged commit fcadc46 into jestjs:master Jun 20, 2018
calebeby referenced this pull request in Pigmice2733/scouting-frontend Jun 30, 2018
This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v23.1.0` to `v23.2.0`



<details>
<summary>Release Notes</summary>

### [`v23.2.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#&#8203;2320)
[Compare Source](jestjs/jest@v23.1.0...v23.2.0)
##### Features

- `[jest-each]` Add support for keyPaths in test titles ([#&#8203;6457](`https://github.com/facebook/jest/pull/6457`))
- `[jest-cli]` Add `jest --init` option that generates a basic configuration file with a short description for each option ([#&#8203;6442](`https://github.com/facebook/jest/pull/6442`))
- `[jest.retryTimes]` Add `jest.retryTimes()` option that allows failed tests to be retried n-times when using jest-circus. ([#&#8203;6498](`https://github.com/facebook/jest/pull/6498`))
##### Fixes

- `[jest-cli]` Add check to make sure one or more tests have run before notifying when using `--notify` ([#&#8203;6495](`https://github.com/facebook/jest/pull/6495`))
- `[jest-cli]` Pass `globalConfig` as a parameter to `globalSetup` and `globalTeardown` functions ([#&#8203;6486](`https://github.com/facebook/jest/pull/6486`))
- `[jest-config]` Add missing options to the `defaults` object ([#&#8203;6428](`https://github.com/facebook/jest/pull/6428`))
- `[expect]` Using symbolic property names in arrays no longer causes the `toEqual` matcher to fail ([#&#8203;6391](`https://github.com/facebook/jest/pull/6391`))
- `[expect]` `toEqual` no longer tries to compare non-enumerable symbolic properties, to be consistent with non-symbolic properties. ([#&#8203;6398](`https://github.com/facebook/jest/pull/6398`))
- `[jest-util]` `console.timeEnd` now properly log elapsed time in milliseconds. ([#&#8203;6456](`https://github.com/facebook/jest/pull/6456`))
- `[jest-mock]` Fix `MockNativeMethods` access in react-native `jest.mock()` ([#&#8203;6505](`https://github.com/facebook/jest/pull/6505`))
##### Chore & Maintenance

- `[docs]` Add jest-each docs for 1 dimensional arrays ([#&#8203;6444](`https://github.com/facebook/jest/pull/6444`/files))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
@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.

jest-each: Support accessing attributes in name interpolation
5 participants