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 a few packages' tests to use Jest Expect (see below) #7579

Merged
merged 7 commits into from Mar 27, 2018

Conversation

@devenbansod
Copy link
Contributor

devenbansod commented Mar 15, 2018

Q                       A
Fixed Issues? Works towards #7476
Patch: Bug Fix? NA
Major: Breaking Change? NA
Minor: New Feature? NA
Tests Added + Pass? Yes
Documentation PR NA
Any Dependency Changes? NA
License MIT
  • Migrate the following packages' tests:

    • babel-helper-annotate-as-pure
    • babel-helper-module-imports
    • babel-helper-transform-fixture-test-runner
    • babel-highlight
    • babel-node
    • babel-plugin-transform-modules-commonjs
    • babel-preset-env-standalone
    • babel-preset-env
    • babel-preset-es2015
    • babel-preset-react
    • babel-standalone
    • babel-template
    • babel-traverse
    • babel-types
  • Some of the fixtures still need to be updated, I'll update them soon

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 15, 2018

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

});
});

describe("when colors are not supported", function() {
stubColorSupport(false);

it("returns false", function() {
assert.ok(!shouldHighlight({}));
expect(!shouldHighlight({})).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

Does something like .toBeFalsy exist?

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 16, 2018

Member

it does

" didn't contain " +
JSON.stringify(expectStdout),
);
expect(includes(stdout, expectStdout)).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

Can we somehow keep the old error message?

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 16, 2018

Member

expect(stdout).stringContaining(expectStdout)

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 16, 2018

Author Contributor

I don't know if stringContaining will work out of the box here, since I believe we are using lodash.includes because stdout is a String in some cases and an object in some.

@@ -48,7 +47,7 @@ describe("inference", function() {

const strictMatch = left.baseTypeStrictlyMatches(right);

assert.ok(!strictMatch, "type might change in if statement");
expect(!strictMatch).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

toBeFalsy()

t.isGenericTypeAnnotation(type) && type.id.name === "T",
"should be T",
);
).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

Can you split this in two assertions?

t.isNumberTypeAnnotation(path.getTypeAnnotation()),
"should be number",
);
expect(t.isNumberTypeAnnotation(path.getTypeAnnotation())).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

Instead of expect(t.isFooBar(node)).toBeTruthy(), can we use expect(node.type).toBe("FooBar")? (Also in other places)

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 16, 2018

Member

Maybe we should add a custom expectation for babel nodes.

expect(node).isNodeOfType("FooBar")? expect(node).matchesNode(t.isFooBar /*, second argument to t.isFooBar */)? https://facebook.github.io/jest/docs/en/expect.html#expectextendmatchers

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 16, 2018

Author Contributor

I tried doing this, but it seems Jest has some problem running with Node4 (works perfectly in Node6+).

We are facing a similar problem with Node4 in adding a new extension to Jest in #7549 too. Any help in this regard would be appreciated.

!getPath("tagged`foo`")
.get("body")[0]
.get("expression")
.isPure(),
);
assert.ok(
).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

.toBeFalsy()

);
assert.strictEqual(
getPath("foo: { }").scope.getLabel("foo").type,
expect(getPath("foo: { }").scope.getBinding("foo")).toBe(undefined);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

.toBeUndefined()

getPath("foo: { }").scope.getLabel("toString"),
undefined,
);
expect(getPath("foo: { }").scope.getLabel("toString")).toBe(undefined);

This comment has been minimized.

Copy link
@nicolo-ribaudo

assert.ok(!traverse.hasType(ast, "ArrowFunctionExpression"));
expect(!traverse.hasType(ast, "ArrowFunctionExpression")).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

.toBeFalsy()

);
expect(() => {
t[k]({ type: "FlavorTownDeclaration" }, {});
}).toThrow();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 15, 2018

Member

Can we test the error message?

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 16, 2018

Member

Yes; you can give a constructor to get an instanceof check, a regexp to test against the message, or a string to === the message (without name).

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect-2 branch from e21b970 to 922ac54 Mar 16, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 16, 2018

Can some one please restart the job at https://travis-ci.org/babel/babel/builds/354156525?utm_source=github_status&utm_medium=notification? This seems to be an unrelated timeout error, but please correct me if I am wrong here.

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect-2 branch from 922ac54 to e3cab74 Mar 16, 2018
@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect-2 branch from 6f72c0f to cbb18aa Mar 24, 2018
devenbansod added 6 commits Mar 24, 2018
* Migrate the following packages' tests:
    * babel-helper-annotate-as-pure
    * babel-helper-module-imports
    * babel-helper-transform-fixture-test-runner
    * babel-highlight
    * babel-node
    * babel-plugin-transform-modules-commonjs
    * babel-preset-env-standalone
    * babel-preset-env
    * babel-preset-es2015
    * babel-preset-react
    * babel-standalone
    * babel-template
    * babel-traverse
    * babel-types
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 24, 2018

Hi, I'm struggling to figure out the cause of the failure at https://travis-ci.org/babel/babel/jobs/357726707#L1144

Moreover, I seem to be getting the same error on re-cloning the project and running the tests on master, but the CI on master seems to be green somehow.

Any help/pointers would be really appreciated.

@@ -11,5 +11,3 @@ export function assertLacksOwnProperty() {}
export function multiline(arr) {
return arr.join("\n");
}

export const assertArrayEquals = assert.deepEqual;

This comment has been minimized.

Copy link
@hzoo

hzoo Mar 26, 2018

Member

technically a breaking change but we didn't intend for anyone to use this package

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

Since this PR will land in Babel 7, I think we could even remove also the empty functions from this file.

@devenbansod You missed another usage of assert in this file 😉

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 27, 2018

Author Contributor

The methods assertHasOwnProperty and assertLacksOwnProperty seem to be used in babel-preset-es2015/test/fixtures/traceur/Classes/InheritanceFromMemberExpression.js and babel-preset-es2015/test/fixtures/traceur/Classes/MethodInheritance.js, though actually since they are empty so are really not testing anything.

Can we replace with following:

export function assertHasOwnProperty() {}

by

export function assertHasOwnProperty(obj, propertyName) {
     expect(obj).toHaveProperty(propertyName);
}

I can't think of functionally equivalent of assertHasNoOwnProperty though. Any ideas?
OR should we just remove the usages of these functions?

@nicolo-ribaudo thanks for pointing that out. Just replaced that usage with expect.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 27, 2018

Member
export function assertHasOwnProperty() {}

by

export function assertHasOwnProperty(obj, propertyName) {
     expect(obj).toHaveProperty(propertyName);
}

You can directly use expect().toHaveProperty() in those tests, without the assertHasOwnProperty function.

I can't think of functionally equivalent of assertHasNoOwnProperty though. Any ideas?
OR should we just remove the usages of these functions?

You can use something like this:

expect(Object.getOwnPropertyNames(obj)).toHaveLength(0);
@hzoo hzoo requested review from existentialism and danez Mar 26, 2018
@@ -59,8 +59,8 @@ class Example {
}


assert(Example.prototype.hasOwnProperty('decoratedProps'));
assert.deepEqual(Example.prototype.decoratedProps, [
expect(Example.prototype.hasOwnProperty('decoratedProps'));

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

This needs the .toBeTruthy call.

assert.ok(new SecondLevel(1) instanceof SecondLevel, 'new SecondLevel is an instanceof SecondLevel');
assert.ok(new SecondLevel(2) instanceof List, 'new SecondLevel is an instanceof List');
assert.ok(new SecondLevel(3) instanceof Array, 'new SecondLevel is an instanceof Array');
expect(new SecondLevel(1) instanceof SecondLevel).toBe(true);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

I think that instead of expect(A instanceof B).toBeTruthy() you can use expect(A).toEqual(expect.any(B)).

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 27, 2018

Author Contributor
@@ -0,0 +1,22 @@
var a = 1;

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

This file should be deleted?

@@ -16,4 +16,4 @@ tally.next(0.1);
tally.next(0.1);
tally.next(0.1);
let last = tally.next("done");
assert.equal(last.value, 0.30000000000000004);
expect(last.value).toBe(0.30000000000000004);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

.toBeCloseTo(0.3, 10)?

assert.equal(typeof s, "symbol");
assert.equal(typeof typeof s.foo, "symbol");
expect(typeof s).toBe("symbol");
expect(typeof typeof s.foo).toBe("symbol");

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

This looks super wrong to me, but it is not related to this PR.

expect(Object.getPrototypeOf(E)).toBe(Function.prototype);
expect(Object.getPrototypeOf(E.prototype)).toBe(Object.prototype);
expect('keys' in E).toBe(false);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

.not.toHaveProperty?

expect(C.hasOwnProperty('5')).toBe(true);
expect(C.hasOwnProperty('9')).toBe(true);
expect(C.hasOwnProperty('10')).toBe(true);
expect(C.hasOwnProperty('11')).toBe(true);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2018

Member

Maybe we could create a custom expect().toHaveOwnProperty() assertion and use it where needed?

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 27, 2018

Author Contributor

I had tried to implement one of our own extensions as mentioned in #7579 (comment) which seems to fail on Node v4. I'm not sure if we can find a workaround (I haven't, in the last few days that I tried to search :-( )

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 27, 2018

Member

I just noticed that expect().toHaveProperty only checks own properties, so it can be used here.
We can try to make expect extensions work later, without blocking this PR.

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Mar 27, 2018

Lgtm so far. That looks like some good hard work there. Thanks.

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect-2 branch from cbb18aa to 4aa3c01 Mar 27, 2018
@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect-2 branch from 4aa3c01 to b09c729 Mar 27, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 27, 2018

So, again Jest throws up a different behavior for not.instanceOf on Node v4 only, while expect(b instanceof Greeting).toBe(false); passes.

Ref to older failed job: https://travis-ci.org/babel/babel/jobs/358974062#L1159

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Mar 27, 2018

Copy link
Member

nicolo-ribaudo left a comment

LGTM 🙂

Copy link
Member

existentialism left a comment

Let's land this

@hzoo
hzoo approved these changes Mar 27, 2018
Copy link
Member

hzoo left a comment

nice work!

@hzoo hzoo merged commit 901571d into babel:master Mar 27, 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.6% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devenbansod devenbansod deleted the devenbansod:migrate-to-jest-expect-2 branch Mar 28, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 28, 2018

Thanks everyone for the patience and support throughout this PR. :-)

@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented on yarn.lock in c8d82d6 Mar 28, 2018

I wonder whether this file should indeed have been a part of the commit?
@nicolo-ribaudo ?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Mar 28, 2018

The yarn.lock it's ok. Thank you for your hard work! :)

@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
7 participants
You can’t perform that action at this time.