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

toBeGreaterThan message correction for failing case #7105

Closed
Abhishek-Modi-Happyperks opened this issue Oct 5, 2018 · 17 comments · Fixed by #7557
Closed

toBeGreaterThan message correction for failing case #7105

Abhishek-Modi-Happyperks opened this issue Oct 5, 2018 · 17 comments · Fixed by #7557

Comments

@Abhishek-Modi-Happyperks
Copy link

Abhishek-Modi-Happyperks commented Oct 5, 2018

Improvement

This bug was
While a case fails for the condition where user expect the result to be greater than certain value , jest reply with the message of expecting a value same as the argument that is passed to function toBeGreaterThan() . It should show something more detailed like >0 or min 1 In case of argument value as 0.

To Reproduce

Steps to reproduce the behavior:
1 Write the function registerUsername as mentioned:-

function registerUsername(username) {
   if (!username) throw new Error('Username is required.');
   return { id: 0, username: username }
}

2 Write a testcase as mentioned:-

describe('registerUser', () => {
    it('should return a user object if valid username is passed', () => {
        const result = lib.registerUser('Abhi')
        expect(result).toMatchObject({ username: 'Abhi'})
        expect(result.id).toBeGreaterThan(0)
    })
})

3 Now run the test.

Expected behavior

> testing-demo@1.0.0 test /var/www/html/11.6- Writing Your First Test/testing-demo
> jest

 FAIL  tests/lib.test.js
  registerUser
    ✕ should return a user object if valid username is passed (11ms)

● registerUser › should return a user object if valid username is passed

expect(received).toBeGreaterThan(expected)

Expected value to be greater than: 0
Received: 0

  51 |         const result = lib.registerUser('Abhi')
  52 |         expect(result).toMatchObject({ username: 'Abhi'})
> 53 |         expect(result.id).toBeGreaterThan(0)
     |                           ^
  54 |     })
  55 | })

  at Object.toBeGreaterThan (tests/lib.test.js:53:27)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 7 passed, 8 total
Snapshots:   0 total
Time:        0.867s, estimated 1s
Ran all test suites.
npm ERR! Test failed.  See above for more details.

Behavior On 23.6.0

> testing-demo@1.0.0 test /var/www/html/11.6- Writing Your First Test/testing-demo
> jest

 FAIL  tests/lib.test.js
  registerUser
    ✕ should return a user object if valid username is passed (11ms)

● registerUser › should return a user object if valid username is passed

expect(received).toBeGreaterThan(expected)

Expected: 0
Received: 0

  51 |         const result = lib.registerUser('Abhi')
  52 |         expect(result).toMatchObject({ username: 'Abhi'})
> 53 |         expect(result.id).toBeGreaterThan(0)
     |                           ^
  54 |     })
  55 | })

  at Object.toBeGreaterThan (tests/lib.test.js:53:27)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 7 passed, 8 total
Snapshots:   0 total
Time:        0.867s, estimated 1s
Ran all test suites.
npm ERR! Test failed.  See above for more details.

Link to repl or repo (highly encouraged)

repl.it demo

The Should Work like this link provided but i am getting different result in 23.6.0

Run npx envinfo --preset jest

Paste the results here:

npx: installed 1 in 2.748s

  System:
    OS: Linux 4.15 Ubuntu 18.04.1 LTS (Bionic Beaver)
    CPU: x64 Intel(R) Core(TM) i3-7100 CPU @ 3.90GHz
  Binaries:
    Node: 8.10.0 - /usr/bin/node
    npm: 6.4.1 - /usr/local/bin/npm

@natealcedo
Copy link
Contributor

natealcedo commented Oct 9, 2018

I don't really see the need for this. The behavior is correct as per what the matcher says: i.e toBeGreaterThan and indeed 0 is not greater than 0. As well, what jest prints to stdout seems developer friendly enough. Maybe you could explain your use case? I'm not sure if we should create a matcher that says toBeGreaterThanOrEqualTo. I imagine that would go into jest-extended. Thoughts? @SimenB @mattphillips

@mattphillips
Copy link
Contributor

toBeGreaterThanOrEqual already exists in Jest core https://jestjs.io/docs/en/expect#tobegreaterthanorequalnumber

I agree that the error message seems pretty clear to me, @Abhishek-Modi-Happyperks would you be able to suggest an alternative error message?

@Abhishek-Modi-Happyperks
Copy link
Author

the message should be as in the expected behavior " Expected value to be greater than " but at the current build its giving " Expected " @mattphillips @natealcedo

@natealcedo
Copy link
Contributor

You have a point I guess. But then again, the matcher is right above the expected and received section. It wouldn't be too difficult to know why the test failed. At the same time, if we were to change to your expected behaviour, you'd have to change similar matchers (toBeLessThan, toBeLessThanOrEqualNumber, etc) to keep developer experience consistent. I don't know. I'd mark this as a quality of life improvement rather than a bug.

@Abhishek-Modi-Happyperks
Copy link
Author

@natealcedo Yes the functions are working properly and it wouldn't be difficult to know. But than at some point one has to figure it out with the stack not with the message. Marked this as Improvement rather than a bug.

@mattphillips
Copy link
Contributor

@Abhishek-Modi-Happyperks I’ve just re-read your example and realised I read your solution as the current behaviour 😆

I’m convinced this is a worthwhile change and should just be a one liner to the message output.

As @natealcedo said we’d need to also check the other similar matchers to fix them too if they need it. Fancy sending a PR?

@Brantron
Copy link

looks like this can be closed based on #7134

@SimenB
Copy link
Member

SimenB commented Oct 15, 2018

I'm open the changing something if it's currently unclear, but not by making it inconsistent. That said, I'm not sure what change would improve it. Dimming the frame? Emphasising the matcher in the error? Having a human readable version of all matchers?

@pedrottimark
Copy link
Contributor

We are working step by step toward two related goals for reports when a matcher fails:

  • remove redundant wording in labels
  • change formatting of matcher name from dim to regular font weight

@natealcedo
Copy link
Contributor

@pedrottimark Is there currently a pull request with the initial work started for this? I'd love to help if the plan is to update matchers one by one.

@SimenB
Copy link
Member

SimenB commented Oct 18, 2018

See #7152

@pedrottimark
Copy link
Contributor

@natealcedo Here are pictures of baseline (at left) and improved (at right) report for toEqual matcher also applies to toStrictEqual matcher. You are welcome to open a pull request with the code changes.

toequal

not-toequal

What took me a while to understand is the ternary expression for message function:

  • function when pass is true is called if matcher is preceded by .not
  • function when pass is false is called if matcher isn’t preceded by .not

@natealcedo
Copy link
Contributor

Sure! I'll take a look at it over the weekend

thymikee pushed a commit that referenced this issue Oct 29, 2018
## Summary

This pull request updates the failure message returned by toStrictEqual so that it is more inline with what #7105 envisions.
 
## Test plan

The matcher is tested with a snapshot test. 

Here are a few screenshots of the before and after along with the test file.

Before: (I had to minimize the image to fit both tests in one screen)

![image](https://user-images.githubusercontent.com/18214059/47253151-16505300-d482-11e8-9596-a5a37c238d4b.png)


After:

![image](https://user-images.githubusercontent.com/18214059/47253131-e012d380-d481-11e8-9211-be3417ec0cd7.png)
@phapp88
Copy link
Contributor

phapp88 commented Nov 2, 2018

I'd love to help by updating one of the other matchers that need to be updated

@pedrottimark
Copy link
Contributor

@phapp88 You are welcome to update toEqual matcher with similar goal as #7224

  • Please don’t refactor as assignment to hint that calls matcherHint even if the test passes.

    const hint = matcherHint('.toEqual', undefined, undefined, {
          isNot: this.isNot,
    }); // please don’t
  • Please do call matcherHint in both callbacks (so it is called only if the test fails) as follows:

    matcherHint('.toEqual', undefined, undefined, {
        isNot: this.isNot,
    }) // please do

@leonaidus
Copy link

v22.1.2 already have the fix.
check this out it works fine:-

https://repl.it/@Abhishek_Modi_H/GrandioseExtralargeMemory

@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
8 participants