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

Migrate `babel-cli` and `babel-generator` tests to use jest-expect #7549

Merged
merged 5 commits into from Apr 11, 2018

Conversation

@devenbansod
Copy link
Contributor

devenbansod commented Mar 11, 2018

Q                       A
Fixed Issues? Working towards #7476
Patch: Bug Fix? NA
Major: Breaking Change? NA
Minor: New Feature? NA
Tests Added + Pass? 👍
Documentation PR NA
Any Dependency Changes? NA
License MIT
  • Migrate babel-cli tests to use jest-expect
  • Migrate babel-generator tests to use jest-expect
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 11, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7497/

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 11, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7202/

chai
.expect(generated)
.ownPropertyDescriptor("map")
.not.to.have.property("value");

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 11, 2018

Member

I guess that this test asserts that generated.map is a getter, since Object.getOwnPropertyDescriptor(generated.map) shouldn't have value?

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 12, 2018

Author Contributor

Oh! It seems I misunderstood. But I'm still unable to figure out a proper test equivalent in Jest expect world. Any suggestions?

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 13, 2018

Author Contributor

@nicolo-ribaudo I feel the new test in Jest world still does test for the scenario you described (at least partly, where I don't think we don't have notion of propertyDescriptor in jest, but otherwise we are still covering everything).

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Apr 3, 2018

Member

It should be expect(Object.getOwnPropertyDescriptor(generated, "map")).not.toHaveProperty("value") instead expect(generated.map).not.toHaveProperty("value")

This comment has been minimized.

Copy link
@devenbansod

devenbansod Apr 3, 2018

Author Contributor

I don't know but this actually fails.

Object.getOwnPropertyDescriptor(generated, "map") returns:

{
        value: {
           version: 3,
           sources: [ 'a.js' ],
           names: [ 'hi', 'msg', 'console', 'log' ],
           mappings: 'AAAA,SAASA,EAAT,CAAaC,GAAb,EAAkB;AAAEC,UAAQC,GAAR,CAAYF,GAAZ;AAAmB'
        },
        writable: true,
        enumerable: true,
        configurable: true
}

while generated.map is:

{
        version: 3,
        sources: [ 'a.js' ],
        names: [ 'hi', 'msg', 'console', 'log' ],
        mappings: 'AAAA,SAASA,EAAT,CAAaC,GAAb,EAAkB;AAAEC,UAAQC,GAAR,CAAYF,GAAZ;AAAmB'
}
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 12, 2018

Also, I don't seem to understand why Jest setup file seems to be failing on Node4 with this error (which is the cause why the Travis build seems to be failing):

 FAIL  packages/babel-generator/test/index.js
  ● Test suite failed to run

    SyntaxError: Unexpected token {

      1 | import toBeType from "jest-tobetype";
      2 | 
    > 3 | jest.setTimeout(10000);
      4 | expect.extend(toBeType);
      5 | 
      
      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:316:17)
      at Object.<anonymous> (test/testSetupFile.js:3:44)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.531s, estimated 3s

@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 13, 2018

Could not find anything definitive on why the testSetupFile.js fails on Node v4, except this says minimum supported version is 6.0.0.

It suggests to possibly use "jest-environment-node" which we are already using at : https://github.com/babel/babel/blob/master/package.json#L100

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch 3 times, most recently from 3ca01fe to 545e774 Mar 28, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 28, 2018

Fixed the tests and not adding the new dependency jest-tobetype. We can later add the jest-tobetype, once we move drop support for Node 4.

const actual = actualFiles[filename];

chai.expect(expect, "Output is missing: " + filename).to.not.be
.undefined;
expect(expected).not.toBe(undefined);

This comment has been minimized.

Copy link
@existentialism

existentialism Mar 28, 2018

Member

Nit: .not.toBeUndefined()

@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Apr 3, 2018

Do let me know if I have missed any action item here. I'd happy to take care of it.

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch from 7e9fcec to b137105 Apr 7, 2018
@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch from b137105 to 90cc745 Apr 7, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Apr 7, 2018

So, then a Green CI! :-)

@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Apr 11, 2018

Can this get in? We have finally got a green CI.
Please let me know if something is missing.

@existentialism existentialism merged commit 9589439 into babel:master Apr 11, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.51% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devenbansod devenbansod deleted the devenbansod:migrate-to-jest-expect branch Apr 12, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Apr 12, 2018

Thanks @existentialism @nicolo-ribaudo for the support!

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Apr 12, 2018

awesome nice work again @devenbansod! 👍

@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.