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

Regression regarding not.toHaveProperty() and non-object input #7942

Closed
ulrichb opened this issue Feb 21, 2019 · 6 comments · Fixed by #7986
Closed

Regression regarding not.toHaveProperty() and non-object input #7942

ulrichb opened this issue Feb 21, 2019 · 6 comments · Fixed by #7986
Assignees

Comments

@ulrichb
Copy link
Contributor

ulrichb commented Feb 21, 2019

💥 Regression Report

.not.toHaveProperty() doesn't work with non-object "actual" values, instead it throws TypeError: Cannot use 'in' operator.

Last working version

Worked up to version: 23.6.0

Stopped working in version: 24.1.0

To Reproduce

expect({ someObj: true }).not.toHaveProperty("isError", true); // Works

// Fails with "Cannot use 'in' operator to search for 'isError' in true" since Jest 24
expect(true).not.toHaveProperty("isError", true);

// Fails with "Cannot use 'in' operator to search for 'isError' in string" since Jest 24
expect("string").not.toHaveProperty("isError", true); // Fails since Jest 24

Expected behavior

No failure in the snippet above.

@pedrottimark
Copy link
Contributor

Confirmed. See #7686 which fixed another regression with unguarded in operator.

The only restriction (by design) on received value of .toHaveProperty or .not.toHaveProperty is to throw matcher error for null or undefined because they cannot have properties.

While testing examples of primitive types in Jest 23.6 especially falsey values, I found (again) an unintended matcher error for false as received value which was fixed in #7241

@ulrichb
Copy link
Contributor Author

ulrichb commented Feb 22, 2019

Thanks for taking a look.

The only restriction (by design) on received value of .toHaveProperty or .not.toHaveProperty is to throw matcher error for null or undefined because they cannot have properties.

For .toHaveProperty() I absolutely agree (throwing on null or undefined). For .not.toHaveProperty() one could discuss if that's the right way.

One example. (In a pre-check) you want to test that it's not an "error response" (an object with an isError == true property). null/undefined fulfills that. So a (theoretical) test would be:

function expectNoErrorResponse(resp) { 
  expect(resp).not.toHaveProperty("isError", true); 
}

//

const response = fetchSomeResponse();

expectNoErrorResponse(response);
expect(response).toBe(null);

Just an idea. I'm not completely against throwing on null/undefined.

@jeysal
Copy link
Contributor

jeysal commented Feb 23, 2019

not.toHaveProperty should also throw a matcher error for nullish values to maintain the symmetry of always throwing when it would also throw without not that I believe all matchers adhere to

@pedrottimark
Copy link
Contributor

pedrottimark commented Feb 23, 2019

It is good to discuss decisions whether matcher fails or throws an error for received value.

For example, we fixed a mistake in #7107 where decision to throw:

  • for expected value was correct
  • for received value seemed consistent, but was incorrect in use cases we overlooked at the time

When a test fails, the report provides relevant information so tester can decide:

  1. is the assertion incorrect (a) especially the expected value, but also (b) assumptions about possible received values computed by code?
  2. why is the received value computed by code incorrect?
  3. are both incorrect?

If test asserts that a property does not exist:

  • If received value computed by code sometimes returns an object and other times returns nullish value, it seems more efficient if matcher returns true.
  • But principle 1b and 2 ask about risk of false positivenegative if matcher returns true: If code returns nullish value (but writer of test assumed it always returns an object) then it seems safer if matcher throws because received value cannot have any property.

What do y’all think?

@ulrichb
Copy link
Contributor Author

ulrichb commented Feb 23, 2019

Hi @pedrottimark,

first I want to thank you for really thinking about what's the best behavior from the viewpoint of an API consumer (a test writer and reader). BTW, I think the symmetry argument from above is not that important (especially because the .not negation is a break of symmetry anyways).


It's a fact that formulating negative assertions is often very fragile (i.e. can lead to false positive assertions due to badly formulated predicates, later test or SUT refactorings, ...). One practice I've learned is to combine negative assertions always with positive assertions (or if this isn't possible with counter-assertions in the arrange phase, or in another counter-example test).

So. From a pure (non test related) API perspective, I personally would not throw for nullish "actual" input because this would be the exact negation of the behavior of the positive "toHaveProperty" assertion. (Of course for any invalid "expected" input it should throw; that's your point 1).
But: From a TDD experience point of view, and the fact that these negative assertions are often fragile anyways, I would recommend throwing for nullish "actual" input, to remove at least null and undefined from the value domain of in this case possibly false positive assertion inputs (or more simple formulated, we reduce the fragility a little bit).

=> It's a trade-off. Overall I'm tended to go for the more conservative way and throw for nullish also in the not.toHaveProperty() case because avoiding fragile tests is more important than the exact logical negation. (But it's a really tough decision.)

@github-actions
Copy link

This issue 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 a pull request may close this issue.

3 participants