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

Improve chai support (with detailed output, to match jest exceptions) #8454

Merged
merged 13 commits into from May 17, 2019
Merged

Improve chai support (with detailed output, to match jest exceptions) #8454

merged 13 commits into from May 17, 2019

Conversation

rpgeeganage
Copy link
Contributor

Summary

This PR is an initial attempt to solve #8152.

Test plan

Before the change Chai output was as follows.

 FAIL  ./chai.spec.js
  a chai test
    ✕ tests something (4ms)

  ● a chai test › tests something

    AssertionError: expected 'hello world' to equal 'hello sunshine'

      3 | describe('a chai test', () => {
      4 |   it('tests something', () => {
    > 5 |     chai.expect('hello world').to.equal('hello sunshine');
        |                                   ^
      6 |   });
      7 | });

      at Object.equal (chai.spec.js:5:35)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.361s
Ran all test suites matching /chai.spec.js/i.

With modification:

  FAIL  ./chai.spec.js
  a chai test
    ✕ tests something (8ms)

  ● a chai test › tests something

    assert.(received, expected)

    Expected value   "hello sunshine"
    Received:
      "hello world"

    Message:
      expected 'hello world' to equal 'hello sunshine'

    Difference:

    - Expected
    + Received

    - hello sunshine
    + hello world

      3 | describe('a chai test', () => {
      4 |   it('tests something', () => {
    > 5 |     chai.expect('hello world').to.equal('hello sunshine');
        |                                   ^
      6 |   });
      7 | });

      at Object.equal (chai.spec.js:5:35)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.534s
Ran all test suites matching /chai.spec.js/i.

@rpgeeganage
Copy link
Contributor Author

@thymikee ,
I have added the tests.
Now still, CI is failing. The failure output is as follows.

$ yarn jest-coverage --color -i --config jest.config.ci.js && yarn test-leak && node scripts/mapCoverage.js && codecov
$ yarn jest --coverage --color -i --config jest.config.ci.js
$ node ./packages/jest-cli/bin/jest.js --coverage --color -i --config jest.config.ci.js
........(node:291) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 exit listeners added. Use emitter.setMaxListeners() to increase limit
.................................................Too long with no output (exceeded 10m0s)

I can't figure out why.
Can you please give me a hint regarding this ?

@jeysal
Copy link
Contributor

jeysal commented May 12, 2019

@rpgeeganage Hmm, I don't see anything suspicious in the diff that might cause tests to stall. Weird that circus passes, must be the change to jasmine2 I guess?
@thymikee seems to be short on time atm, maybe @SimenB has an idea?
Your e2e test passes locally for me as well...

@jeysal
Copy link
Contributor

jeysal commented May 12, 2019

Maybe it broke some other test that waits for a certain output and for some reason doesn't timeout normally? @rpgeeganage have you tried running all tests (yarn jest) locally with your changes? (Will take a looooong time unless you're on Linux)

@rpgeeganage
Copy link
Contributor Author

@jeysal
Thanks a lot. I'll dig deep.

@SimenB
Copy link
Member

SimenB commented May 12, 2019

Looks like some sort of hanging test, yes. Not sure why it doesn't time out, probably a bug.

Running yarn jest locally is a good idea :)

@rpgeeganage
Copy link
Contributor Author

@SimenB , @jeysal
I tried and I think I located the error. It's in https://github.com/facebook/jest/blob/master/e2e/failures/__tests__/duringTests.test.js#L18
I think it is because of a bug in my modification. I'm looking into it now.
Thanks a lot.

@codecov-io
Copy link

codecov-io commented May 12, 2019

Codecov Report

Merging #8454 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8454      +/-   ##
==========================================
- Coverage   62.32%   62.26%   -0.07%     
==========================================
  Files         266      266              
  Lines       10735    10746      +11     
  Branches     2614     2624      +10     
==========================================
  Hits         6691     6691              
- Misses       3460     3471      +11     
  Partials      584      584
Impacted Files Coverage Δ
...ackages/jest-jasmine2/src/assertionErrorMessage.ts 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/Spec.ts 0% <0%> (ø) ⬆️
packages/jest-circus/src/formatNodeAssertErrors.ts 12.96% <0%> (-1.33%) ⬇️
packages/jest-jasmine2/src/jasmine/Env.ts 0% <0%> (ø) ⬆️

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 15e2cc6...de3815f. Read the comment docs.

@rpgeeganage
Copy link
Contributor Author

@jeysal ,
I have fixed the comments. Please review the new change.
Thanks a lot.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Really nice work @rpgeeganage ❤️
Hope the change to chai also goes through for an even better integration 😎

@jeysal jeysal merged commit 6d31fb1 into jestjs:master May 17, 2019
@rpgeeganage
Copy link
Contributor Author

@jeysal ,
Thanks a lot and I'm honored to contribute to this project.
I'm planning to dive into possible chai modification during the weekend.

@rpgeeganage rpgeeganage deleted the improve_chai_assertion_messages_display branch May 17, 2019 16:45
@lucasfcosta
Copy link
Contributor

Hello everyone, I've just seen the related issue in Chai's repo, I'll do my best to get it merged there this week. I think the code is pretty much g2g.

@lucasfcosta
Copy link
Contributor

Just FYI, we've just merged @rpgeeganage's amazing work in chaijs/chai#1257 and we'll try to separate some time for a release with that change this weekend.

@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 11, 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.

None yet

6 participants