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) #8444

Closed
wants to merge 36 commits into from
Closed

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

wants to merge 36 commits into from

Conversation

rpgeeganage
Copy link
Contributor

@rpgeeganage rpgeeganage commented May 9, 2019

Summary

This PR is an initial attempt to solve #8152.

Test plan

No tests were added yet since this only for review.

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.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Nice! Can you add a test to make sure we don't regress? And a changelog entry. Also, how about porting this fix to jest-circus?

@jeysal
Copy link
Contributor

jeysal commented May 9, 2019

Hey @rpgeeganage, thanks a lot for tackling this!
To expand a bit on @thymikee's circus comment:
jest-circus is a replacement for the old jest-jasmine2, and it is to become the default soon. This is why I agree that we'll need this in circus as well so we don't lose the functionality when changing the default. You can set JEST_CIRCUS=1 in the environment to switch to circus manually. If you need any help with circus or the tests, feel free to ask us! :)

@rpgeeganage
Copy link
Contributor Author

@thymikee , @jeysal ,
Sorry for the late reply. Thanks a lot for the feedback.
I'll look into the jest-circus. I was not confident enough about the solution and that's why I opened the PR for review.
I'll add tests and look into the jest-circus.
Thanks again.

@rpgeeganage
Copy link
Contributor Author

@jeysal @thymikee
I have introduced the same fix for circus.
I have check the output as follows.

JEST_CIRCUS=1 node ../../jest_unit/jest/packages/jest-cli/bin/jest.js chai.spec.js
 FAIL  ./chai.spec.js
  a chai test
    ✕ tests something (4ms)

  ● 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.389s
Ran all test suites matching /chai.spec.js/i.

Can you give me a hint on adding tests for these changes? (where or how to add the tests)
Thanks a lot.

@thymikee
Copy link
Collaborator

I think the easiest would be to add e2e tests that run whole jest on a specified set of files. They sit in e2e directory, just try to mimic existing ones :)

@rpgeeganage
Copy link
Contributor Author

@thymikee ,
Thanks a lot. I'll add a new test there.

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

@rpgeeganage
Copy link
Contributor Author

closing this pr, because I screwed it up. will open a new PR. sorry about that.

@rpgeeganage
Copy link
Contributor Author

#8454

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

5 participants