expect: Fix non-object received value in toHaveProperty

[pedrottimark]

Summary

Fixes #7942

1. In getPath function, EDIT moveguard in operator into try blockwith conditional and for primitive types boolean-number-string-symbol and use it only if value is undefined
2. Although not a bug in the issue, also fix false negative from incorrect logic in hasOwnProperty method that called equals for value undefined even when property does not exist in received object

Residue for future pull requests:

• Replace obscure 'pass must be initialized' error with meaningful matcher error if expected array path is empty
• Rewrite getPath from recursive to iterative
• Improve report when test fails

Test plan

Added 14 test cases. Here is the key to notes in cells:

1. in was broken in Jest 24: TypeError: Cannot use 'in' operator to search for 'key' in …
2. false negative has been passing when it should fail: Expected the function to throw an error. But it didn't throw anything.
3. prototype was fixed in Jest 24: TypeError: Cannot read property 'prototype' of undefined
4. false positive was incorrect error for false as received value

The other tests confirm that toHaveProperty matcher is compatible with in operator.

For example, the first row that passes for a setter without a getter. It is what it is :(

received	path	value	23.6.0	24.0.0
new Foo()	'setter'	undefined
foo2	'a'	undefined
foo2	'c'	'c'
foo2	'val'	true
new E()	'nodeType'	1
''	'length'	0		in
memoized	'memo'	[]
{a: {}}	'a.b'	undefined	false negative	false negative
false	key		false positive	in
0	key			in
''	key			in
Symbol()	key			in
Object.create	'not'		prototype
{}	[]

[pedrottimark]

cc @ulrichb @jeysal @dubzzz

[jeysal]

Can't say I completely understand what is going on in this matcher, but the added tests all seem correct to me. Just unfortunate that the snapshot names are getting a bit confusing because for many of them, the reason for the assertion result is not visible in the stringified obj 😐

[pedrottimark]

@jeysal Yeah, although concise parallel arrays of test cases seem super at first glance:

• hard to distinguish obvious original spec and subtle later regression
• hard to match up with serialized title of snapshots, especially with instances

After enough time to be confident that the change in behavior is solid, when we collaborate to improve the report, let’s pay special attention to clear feedback when received is an instance.

[dubzzz]

As I was on the review, I just did it ;)
Looks good to me

[ulrichb]

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).

[pedrottimark]

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?

• While preparing to rewrite with typeof I found either object or function (which was reason to add memoized test case)
• While writing this comment, I noticed yet another possibility of Host object (provided by the JS environment) while rereading https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof#Description

[ulrichb]

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).

[pedrottimark]

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

[ulrichb]

Okay, great! And thanks for fixing this issue!

[SimenB]

This needs #7708...

[SimenB]

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

[pedrottimark]

Thank you. I just pushed another refactoring commit :)

[mjesun]

This is, BTW, a breaking change. Multiple tests are failing due to:

received	path	value	23.6.0	24.0.0
{a: {}}	'a.b'	undefined	false negative	false negative

I thought the standard way of behaving for Jest was to consider key: undefined and the absence of the key itself the same.

[SimenB]

I thought the standard way of behaving for Jest was to consider key: undefined and the absence of the key itself the same.

Yeah mostly, but we want to move away from it (why toStrictEqual is a thing). Should not break in a minor of course.

[mjesun]

Makes sense, I agree with the change. Should we back-out for the minor? Or is it good to go as-is?

[SimenB]

I think we should make sure it works like it does in 24.1.0 (it is a breaking change), and create an issue for reinstating the current behavior for Jest 25. Agreed?

We still want to fix #7942, though

[pedrottimark]

@mjesun The purpose of the pull request was to fix a regression. I might have gone too far.

To help me figure out if we can keep the unbreaking part and defer the breaking part, can you give a minimal example of assertion that fails?

[pedrottimark]

Aha, the bug in assignment to pass which caused contradictory results:

expect({a: {}}).toHaveProperty('a.b', undefined); // 23.6
expect({a: {}}).not.toHaveProperty('a.b'); // 23.6

Yeah, I will open pull request to change it back and comment out the test until Jest 25

[github-actions]

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.