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-core tests to use jest-expect #7513

Merged
merged 1 commit into from Mar 10, 2018

Conversation

@devenbansod
Copy link
Contributor

devenbansod commented Mar 6, 2018

Q                       A
Fixed Issues? Working towards #7476
Patch: Bug Fix? NA
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR NA
Any Dependency Changes? NA
License MIT
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 6, 2018

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

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 6, 2018

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

import path from "path";
import Plugin from "../lib/config/plugin";
import generator from "@babel/generator";

function assertIgnored(result) {
assert.ok(!result);
expect(!result).toBeTruthy();

This comment has been minimized.

Copy link
@Andarist

Andarist Mar 6, 2018

Member

this probably should at least get reversed to

expect(result).toBeFalsy();

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 7, 2018

Author Contributor

I agree. :-) Will make the change in sometime.

}

function assertNotIgnored(result) {
assert.ok(!result.ignored);
expect(!result.ignored).toBeTruthy();

This comment has been minimized.

Copy link
@Andarist

Andarist Mar 6, 2018

Member

same here

babel.transform(string, { ast: true }).ast,
);
assert.equal(newTransform(string).code, string);
expect(newTransform(string).code).toEqual(string);

This comment has been minimized.

Copy link
@Andarist

Andarist Mar 6, 2018

Member

.code is a string so imho .toBe would be more appropriate here (and in other places where .code is tested)

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 7, 2018

Author Contributor

I believe I messed up in applying the codemod (meaning I applied the code-mod to an already modified file. These were supposed to be correctly changed to toBe according to the code-mod (if I had applied it correctly).
Changing now.

@@ -608,25 +596,25 @@ describe("api", function() {

it("all", function() {
const script = babel.buildExternalHelpers();
assert.ok(script.indexOf("classCallCheck") >= -1);
assert.ok(script.indexOf("inherits") >= 0);
expect(script.indexOf("classCallCheck") >= -1).toBeTruthy();

This comment has been minimized.

Copy link
@Andarist

Andarist Mar 6, 2018

Member

you could use arrayContaining matcher here

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 7, 2018

Author Contributor

I am not sure if we can use it here, since typeof script returns string.

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 7, 2018

Member

You can if you use .split(/\b/) first :trollface:

Or use the stringContaining matcher 🤷‍♀️

Replacing indexOfs would need to be a case-by-case thing, not really something a codemod can do without type information 😞

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 7, 2018

Author Contributor

Agreed. Thanks @Kovensky for the suggestion.
Now, changed to using stringContaining. :-)

Copy link
Contributor Author

devenbansod left a comment

Thanks @Andarist for the review.

babel.transform(string, { ast: true }).ast,
);
assert.equal(newTransform(string).code, string);
expect(newTransform(string).code).toEqual(string);

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 7, 2018

Author Contributor

I believe I messed up in applying the codemod (meaning I applied the code-mod to an already modified file. These were supposed to be correctly changed to toBe according to the code-mod (if I had applied it correctly).
Changing now.

@@ -608,25 +596,25 @@ describe("api", function() {

it("all", function() {
const script = babel.buildExternalHelpers();
assert.ok(script.indexOf("classCallCheck") >= -1);
assert.ok(script.indexOf("inherits") >= 0);
expect(script.indexOf("classCallCheck") >= -1).toBeTruthy();

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 7, 2018

Author Contributor

I am not sure if we can use it here, since typeof script returns string.

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch 2 times, most recently from 76f246c to 9e1c1c7 Mar 7, 2018
@danez

This comment has been minimized.

Copy link
Member

danez commented Mar 7, 2018

In fixtures tests, we could make both available expect and assert, so we don't have to migrate everything at once.
I guess it should work if we forward expect here, but haven't tested:

https://github.com/babel/babel/blob/master/packages/babel-helper-transform-fixture-test-runner/src/index.js#L19..L25

});

it("code option false", function() {
return transformAsync("foo('bar');", { code: false }).then(function(
result,
) {
assert.ok(!result.code);
expect(!result.code).toBeTruthy();

This comment has been minimized.

Copy link
@danez

danez Mar 7, 2018

Member

Could be expect(result.code).toBeFalsy();

});
});

it("ast option false", function() {
return transformAsync("foo('bar');", { ast: false }).then(function(result) {
assert.ok(!result.ast);
expect(!result.ast).toBeTruthy();

This comment has been minimized.

Copy link
@danez

danez Mar 7, 2018

Member

Could be expect(result.ast).toBeFalsy();

});
});

it("ast option default", function() {
return transformAsync("foo('bar');").then(function(result) {
assert.ok(!result.ast);
expect(!result.ast).toBeTruthy();

This comment has been minimized.

Copy link
@danez

danez Mar 7, 2018

Member

Could be expect(result.ast).toBeFalsy();

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch 2 times, most recently from 6484861 to 4e3d87c Mar 7, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 7, 2018

Thanks @danez for the review. Adding expect at your suggested location did help. Have added those changes with the latest squashed commit.

Only thing missing to be migrated in babel-core's test is packages/babel-core/test/parse.js. It seems to have been added in 22c8f63#diff-a9a30dd3ba728e2fa262f32730241c5f but I am not sure what is the test meant to assert here. Any pointers?

@mallikarjuna91

This comment has been minimized.

Copy link

mallikarjuna91 commented Mar 7, 2018

@devenbansod The test is meant to assert the Parsed input file matches the output file ?

@danez

This comment has been minimized.

Copy link
Member

danez commented Mar 7, 2018

I guess these should be toBe or toEqual. They just compare if the result of parse is equal to the content of output.json.

@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 7, 2018

Because here it says it's an alias of assert.ok() which just takes in value and message and tests whether the value is truthy. It did not look like that file assertion. May be the author meant to add assert.equal or assert.deepEqual but forgot?

According to your suggestion, I have changed to:

function fixture(...args) {
  return path.join(__dirname, "fixtures", "parse", ...args);
}

describe("parse", function() {
  it("should parse using configuration from .babelrc when a filename is provided", function() {
    const input = fs.readFileSync(fixture("input.js"), "utf8");
    const output = require(fixture("output"));
    expect(
      parse(input, { filename: fixture("input.js"), cwd: fixture() }),
    ).toEqual(output);
  });

  it("should parse using passed in configuration", function() {
    const input = fs.readFileSync(fixture("input.js"), "utf8");
    const output = require(fixture("output"));
    expect(parse(input, { parserOpts: { plugins: ["decorators"] } })).toEqual(
      output,
    );
  });
});

I seem to be getting errors mentioned in this gist. Is there something that I am missing here? Apologies for repeated questions and queries.

@danez

This comment has been minimized.

Copy link
Member

danez commented Mar 7, 2018

Okay seems the test never worked as intended.
I think it could work when you change it to:

...
const result = parse(...);
expect(JSON.parse(JSON.stringify(result))).toEqual(output);
...

If it doesn't work this way, then you can also simply check for truthy now but leave a comment that says it should be fixed. We can then create a separate ticket to fix the tests.

@kaicataldo We might need to do something more clever here anyway unless we really want to snapshot the order of the fields. Babylon has this compare function maybe we should share it and use it here too.

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch 2 times, most recently from cf22317 to 35470ca Mar 7, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 7, 2018

Updated the tests.
I believe all tests assertions in babel-core have been moved to jest expect. (Please point out if some have been left still)

@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 9, 2018

Not that I want to force this through (I understand maintainers are generally over-crowded with tasks). Please let me know if some action item is still pending and needs to be addressed to get this through.

@@ -386,48 +376,46 @@ describe("api", function() {
" _classCallCheck(this, Foo);",
"};",
].join("\n"),
result.code,
);
).toEqual(result.code);

This comment has been minimized.

Copy link
@nicolo-ribaudo
RegExp(
"Support for the experimental syntax 'dynamicImport' isn't currently enabled \\(1:9\\):",
).exec(err.message),
);
assert.ok(
).toBeTruthy();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 9, 2018

Member

Here and in the next assertions you can use expect(err.message).toMatch("Support for the experimental syntax 'dynamicImport' isn't currently enabled (1:9):).

assert.equal(opts1.plugins.length, 1);
assert.equal(opts2.plugins.length, 1);
assert.notEqual(opts1.plugins[0], opts2.plugins[1]);
expect(opts1.plugins.length).toBe(1);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 9, 2018

Member

.toHaveLength(1)? (Also in different places in this file)

@@ -2,5 +2,5 @@ function* foo(bar = "bar") {
return bar;
}

assert.deepEqual(foo().next().value, "bar");
assert.deepEqual(foo("foo").next().value, "foo");
expect(foo().next().value).toEqual("bar");

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 9, 2018

Member

.toBe?

filename: fixture("input.js"),
cwd: fixture(),
});
expect(JSON.parse(JSON.stringify(result))).toEqual(output);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 9, 2018

Member

Why is JSON.parse(JSON.stringify()) needed?

This comment has been minimized.

Copy link
@danez

danez Mar 9, 2018

Member

Because the returned AST contains classes, whereas the json file just has objects.

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 9, 2018

Author Contributor

@danez had suggested it to fix the errors described here.

@@ -22,6 +22,7 @@ const testContext = vm.createContext({
transform: babel.transform,
setTimeout: setTimeout,
setImmediate: setImmediate,
expect,

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 9, 2018

Member

Can you also use expect in ./helpers?

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 9, 2018

Author Contributor

I had left it to be migrated when we are in a stage to completely migrate away from assert. Anyway, at most what I can change directly is assertNoOwnProperties. We can't migrate the other helper assertArrayEquals without running the code-mod at related places (I plan to do it a separate PR).

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch from af3ad38 to 604fef5 Mar 9, 2018
@@ -384,7 +383,7 @@ describe("buildConfigChain", function() {
comments: true,
});

assert.equal(opts.comments, undefined);

This comment has been minimized.

Copy link
@danez

danez Mar 9, 2018

Member

toBe(undefined) could be toBeUndefined()
also in other places

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 9, 2018

Author Contributor

Changed. (Y)

@@ -533,7 +532,7 @@ describe("buildConfigChain", function() {
fixture("nonexistant-fake", "other.js"),
],
});
assert.equal(opts1, null);

This comment has been minimized.

Copy link
@danez

danez Mar 9, 2018

Member

toBe(null) could be toBeNull()
also in other places

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 9, 2018

Author Contributor

Changed. Thanks for pointing it out.

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch from 604fef5 to 9a39fe0 Mar 9, 2018

// 2. passPerPreset: false

aliasBaseType = null;

result = execTest(false);

assert.equal(aliasBaseType, null);
expect(aliasBaseType).toBe(null);

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 9, 2018

Author Contributor

Sorry, missed it here. Changing.

@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch from 9a39fe0 to c75feb8 Mar 9, 2018
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 9, 2018

I am not sure if the failure at https://travis-ci.org/babel/babel/jobs/351350222 is related. Can someone re-trigger it?

assert.equal(0, options.presets.length);
expect(true).toBe(Array.isArray(options.plugins));
expect(1).toBe(options.plugins.length);
expect(0).toBe(options.presets.length);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 9, 2018

Member

Can you reverse those conditions? (And use the appropriate expect().* functions)

Sorry for not spotting this in my earlier review.

This comment has been minimized.

Copy link
@devenbansod

devenbansod Mar 9, 2018

Author Contributor

Added these changes. Thanks for pointing out.

* Used codemods at: https://gist.github.com/devenbansod/03c5cff857661e076cbec72fcb2e7eb3 along with some manual intervention and review
@devenbansod devenbansod force-pushed the devenbansod:migrate-to-jest-expect branch from c75feb8 to 2622383 Mar 9, 2018
@danez
danez approved these changes Mar 10, 2018
@danez danez merged commit f3f0197 into babel:master Mar 10, 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.73% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devenbansod

This comment has been minimized.

Copy link
Contributor Author

devenbansod commented Mar 10, 2018

Thanks a lot for your patience and support throughout this PR @danez @nicolo-ribaudo :-)

@devenbansod devenbansod deleted the devenbansod:migrate-to-jest-expect branch Mar 10, 2018
@danez

This comment has been minimized.

Copy link
Member

danez commented Mar 10, 2018

Thank you. Looking forward to the next PRs :)

aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
@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.