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.

Sign up for GitHub

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

expect: Fix non-object received value in toHaveProperty #7986

Merged
merged 6 commits into from
Feb 28, 2019
Merged

expect: Fix non-object received value in toHaveProperty #7986

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7049862
expect: Fix non-object received value in toHaveProperty
pedrottimark Feb 25, 2019
08d3066
Update CHANGELOG.md
pedrottimark Feb 25, 2019
b9aa00b
Replace try-catch with typeof and conditional and
pedrottimark Feb 26, 2019
04c82d0
Resolve merge conflict in CHANGELOG.md
pedrottimark Feb 28, 2019
a65dc2c
import isPrimitive from jest-get-type
pedrottimark Feb 28, 2019
538dbcc
Rewrite if-else as logical expression
pedrottimark Feb 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
- `[jest-worker]` Fix `jest-worker` when using pre-allocated jobs ([#7934](https://github.com/facebook/jest/pull/7934))
- `[jest-changed-files]` Fix `getChangedFilesFromRoots` to not return parts of the commit messages as if they were files, when the commit messages contained multiple paragraphs ([#7961](https://github.com/facebook/jest/pull/7961))
- `[static]` Remove console log '-' on the front page
- `[expect]` Fix non-object received value in toHaveProperty ([#7986](https://github.com/facebook/jest/pull/7986))

### Chore & Maintenance

Expand Down
149 changes: 149 additions & 0 deletions packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2773,6 +2773,8 @@ exports[`.toHaveProperty() {error} expect({"a": {"b": {}}}).toHaveProperty('unde
Expected has value: <green>undefined</>"
`;

exports[`.toHaveProperty() {error} expect({}).toHaveProperty('') 1`] = `"pass must be initialized"`;

exports[`.toHaveProperty() {error} expect(null).toHaveProperty('a.b') 1`] = `
"<dim>expect(</><red>received</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expand All @@ -2789,6 +2791,16 @@ exports[`.toHaveProperty() {error} expect(undefined).toHaveProperty('a') 1`] = `
Received has value: <red>undefined</>"
`;

exports[`.toHaveProperty() {pass: false} expect("").toHaveProperty('key') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expected the object:
<red>\\"\\"</>
To have a nested property:
<green>\\"key\\"</>
"
`;

exports[`.toHaveProperty() {pass: false} expect("abc").toHaveProperty('a.b.c') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expand Down Expand Up @@ -2914,6 +2926,19 @@ Difference:
Comparing two different types of values. Expected <green>undefined</> but received <red>number</>."
`;

exports[`.toHaveProperty() {pass: false} expect({"a": {}}).toHaveProperty('a.b', undefined) 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>{\\"a\\": {}}</>
To have a nested property:
<green>\\"a.b\\"</>
With a value of:
<green>undefined</>
Received:
<red>object</>.a: <red>{}</>"
`;

exports[`.toHaveProperty() {pass: false} expect({"a": 1}).toHaveProperty('a.b.c.d') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expand Down Expand Up @@ -2963,6 +2988,16 @@ Received:
<red>1</>"
`;

exports[`.toHaveProperty() {pass: false} expect({"key": 1}).toHaveProperty('not') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expected the object:
<red>{\\"key\\": 1}</>
To have a nested property:
<green>\\"not\\"</>
"
`;

exports[`.toHaveProperty() {pass: false} expect({}).toHaveProperty('a') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expand Down Expand Up @@ -3019,6 +3054,16 @@ Difference:
Comparing two different types of values. Expected <green>undefined</> but received <red>string</>."
`;

exports[`.toHaveProperty() {pass: false} expect(0).toHaveProperty('key') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expected the object:
<red>0</>
To have a nested property:
<green>\\"key\\"</>
"
`;

exports[`.toHaveProperty() {pass: false} expect(1).toHaveProperty('a.b.c') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expand All @@ -3041,6 +3086,50 @@ With a value of:
"
`;

exports[`.toHaveProperty() {pass: false} expect(Symbol()).toHaveProperty('key') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expected the object:
<red>Symbol()</>
To have a nested property:
<green>\\"key\\"</>
"
`;

exports[`.toHaveProperty() {pass: false} expect(false).toHaveProperty('key') 1`] = `
"<dim>expect(</><red>object</><dim>).toHaveProperty(</><green>path</><dim>)</>

Expected the object:
<red>false</>
To have a nested property:
<green>\\"key\\"</>
"
`;

exports[`.toHaveProperty() {pass: true} expect("").toHaveProperty('length', 0) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>\\"\\"</>
Not to have a nested property:
<green>\\"length\\"</>
With a value of:
<green>0</>
"
`;

exports[`.toHaveProperty() {pass: true} expect([Function memoized]).toHaveProperty('memo', []) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>[Function memoized]</>
Not to have a nested property:
<green>\\"memo\\"</>
With a value of:
<green>[]</>
"
`;

exports[`.toHaveProperty() {pass: true} expect({"a": {"b": [1, 2, 3]}}).toHaveProperty('a,b,1') 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>)</>

Expand Down Expand Up @@ -3185,6 +3274,18 @@ With a value of:
"
`;

exports[`.toHaveProperty() {pass: true} expect({"nodeName": "DIV"}).toHaveProperty('nodeType', 1) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>{\\"nodeName\\": \\"DIV\\"}</>
Not to have a nested property:
<green>\\"nodeType\\"</>
With a value of:
<green>1</>
"
`;

exports[`.toHaveProperty() {pass: true} expect({"property": 1}).toHaveProperty('property', 1) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expand All @@ -3197,6 +3298,42 @@ With a value of:
"
`;

exports[`.toHaveProperty() {pass: true} expect({"val": true}).toHaveProperty('a', undefined) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>{\\"val\\": true}</>
Not to have a nested property:
<green>\\"a\\"</>
With a value of:
<green>undefined</>
"
`;

exports[`.toHaveProperty() {pass: true} expect({"val": true}).toHaveProperty('c', "c") 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>{\\"val\\": true}</>
Not to have a nested property:
<green>\\"c\\"</>
With a value of:
<green>\\"c\\"</>
"
`;

exports[`.toHaveProperty() {pass: true} expect({"val": true}).toHaveProperty('val', true) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>{\\"val\\": true}</>
Not to have a nested property:
<green>\\"val\\"</>
With a value of:
<green>true</>
"
`;

exports[`.toHaveProperty() {pass: true} expect({}).toHaveProperty('a', undefined) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expand All @@ -3221,6 +3358,18 @@ With a value of:
"
`;

exports[`.toHaveProperty() {pass: true} expect({}).toHaveProperty('setter', undefined) 1`] = `
"<dim>expect(</><red>object</><dim>).not.toHaveProperty(</><green>path</><dim>, </><green>value</><dim>)</>

Expected the object:
<red>{}</>
Not to have a nested property:
<green>\\"setter\\"</>
With a value of:
<green>undefined</>
"
`;

exports[`.toMatch() {pass: true} expect(Foo bar).toMatch(/^foo/i) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toMatch(</><green>expected</><dim>)</>

Expand Down
33 changes: 33 additions & 0 deletions packages/expect/src/__tests__/matchers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,7 +1206,26 @@ describe('.toHaveProperty()', () => {
get b() {
return 'b';
}
set setter(val) {
this.val = val;
}
}

class Foo2 extends Foo {
get c() {
return 'c';
}
}
const foo2 = new Foo2();
foo2.setter = true;

function E(nodeName) {
this.nodeName = nodeName.toUpperCase();
}
E.prototype.nodeType = 1;

const memoized = function() {};
memoized.memo = [];

[
[{a: {b: {c: {d: 1}}}}, 'a.b.c.d', 1],
Expand All @@ -1219,6 +1238,13 @@ describe('.toHaveProperty()', () => {
[Object.assign(Object.create(null), {property: 1}), 'property', 1],
[new Foo(), 'a', undefined],
[new Foo(), 'b', 'b'],
[new Foo(), 'setter', undefined],
[foo2, 'a', undefined],
[foo2, 'c', 'c'],
[foo2, 'val', true],
[new E('div'), 'nodeType', 1],
['', 'length', 0],
[memoized, 'memo', []],
].forEach(([obj, keyPath, value]) => {
test(`{pass: true} expect(${stringify(
obj,
Expand All @@ -1245,6 +1271,7 @@ describe('.toHaveProperty()', () => {
[{a: {b: {c: 5}}}, 'a.b', {c: 4}],
[new Foo(), 'a', 'a'],
[new Foo(), 'b', undefined],
[{a: {}}, 'a.b', undefined],
].forEach(([obj, keyPath, value]) => {
test(`{pass: false} expect(${stringify(
obj,
Expand Down Expand Up @@ -1280,6 +1307,11 @@ describe('.toHaveProperty()', () => {
[{}, 'a'],
[1, 'a.b.c'],
['abc', 'a.b.c'],
[false, 'key'],
[0, 'key'],
['', 'key'],
[Symbol(), 'key'],
[Object.assign(Object.create(null), {key: 1}), 'not'],
].forEach(([obj, keyPath]) => {
test(`{pass: false} expect(${stringify(
obj,
Expand All @@ -1297,6 +1329,7 @@ describe('.toHaveProperty()', () => {
[{a: {b: {}}}, undefined],
[{a: {b: {}}}, null],
[{a: {b: {}}}, 1],
[{}, []], // Residue: pass must be initialized
].forEach(([obj, keyPath]) => {
test(`{error} expect(${stringify(
obj,
Expand Down
6 changes: 3 additions & 3 deletions packages/expect/src/matchers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,9 +602,9 @@ const matchers: MatchersObject = {
const result = getPath(object, keyPath);
const {lastTraversedObject, hasEndProp} = result;

const pass = valuePassed
? equals(result.value, value, [iterableEquality])
: hasEndProp;
const pass =
hasEndProp &&
(!valuePassed || equals(result.value, value, [iterableEquality]));

const traversedPath = result.traversedPath.join('.');

Expand Down
17 changes: 16 additions & 1 deletion packages/expect/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,22 @@ export const getPath = (
result.traversedPath.unshift(prop);

if (lastProp) {
result.hasEndProp = prop in object;
if (newObject === undefined) {
// Does object have the property with an undefined value?
// At this point, object cannot be null nor undefined.
// Although primitive values support bracket notation (above)
// they would throw TypeError for in operator (below).
const type = typeof object;
result.hasEndProp =
type !== 'boolean' &&
type !== 'number' &&
type !== 'string' &&
type !== 'symbol' &&
prop in object;
} else {
result.hasEndProp = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In case the try/catch is only about primitives, I would prefer ... && typeof object === "object" && prop in object over try/catch.

In case of other situations, where in would throw where we really have no alternative to try/catch, then I would prefer extending the comment in the catch-clause to make it clear for the next code reader (and prevent her/him refactoring to a typeof x === "object" check).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for constructive critique from viewpoint of reader. Yes, I agree that the comment is incomplete and unclear.

Here is some data. What do you suggest to improve either code or comment, or both?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I would prefer to avoid try/catch and instead typeof-check for object/function (or maybe negative check for number/string/boolean/symbol).

Reasoning: Here have to swallow/mask a thrown error completely (it wouldn't be possible to e.g. log it). In such situations it's more safe to just state all the expected exemptions than possibly hide non-in operator related (i.e. really unexpected) errors. If we later need some additional exemptions (e.g. for your mentioned Host object or whatever) just add them later (+ fixate them with test cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. Thank you for clear and convincing reasoning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, great! And thanks for fixing this issue!

Copy link
Member

Choose a reason for hiding this comment

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

This needs #7708...

Copy link
Member

Choose a reason for hiding this comment

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

Merged that, could we use import {isPrimitive} from 'jest-get-type';? And if that's insufficient, fix it so it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I just pushed another refactoring commit :)


if (!result.hasEndProp) {
result.traversedPath.shift();
}
Expand Down