Jasmine/Jest v20+: this syntax codemod

[avaly]

Closes #57

Context: jestjs/jest#3553

[skovhus]

@avaly thank you so much for contributing. 👍🏻

One thing I would like to figure out is how this new transformer fits with some of the other transformations. Example: when using Mocha, I guess it would make sense to run your transformer as part of the migration. So it this new transformer safe enough to run as part of the other codemods?

Let me know what you think. I'll review the code soon. : )

[avaly]

@skovhus This codemod assumes the source code already matches Jest's syntax (describe/before/beforeEach/it). So I would say it's safe to run as part of a migration as long as it's run as the last step in the process, after all the other codemods have converted the ava/mocha/tape syntax to Jest. I would say it's especially useful for Mocha migrations, since Mocha uses this for sharing context among tests.

[avaly]

Support also test

[skovhus]

Thanks for this! Looks good to me. : )

I think it might make sense to run this after the Mocha transformer, similar to how we run chai-should as part of should.js here https://github.com/skovhus/jest-codemods/blob/master/src/transformers/should.js#L89

[skovhus]

I'm wondering a bit if we can come up with a more descriptive name... Should we call it "Jasmine: this usage"? Hm.

[skovhus]

Jasmine

[skovhus]

I would just do .sort().reverse()

[skovhus]

Can I get you to rebase master and use the new finale instead of quoting detection? See https://github.com/skovhus/jest-codemods/pull/39/files#diff-80cc69d0b8fb32fd19621a13564a36eeL218

[skovhus]

Be aware that Mocha has some magic this functions (see #56)

[avaly]

@skovhus I've address your comments, rebased, and added the special Mocha functions to an ignore list.

[skovhus]

Awesome work! Will hopefully be able to release this tomorrow.

[quantizor]

Timeout actually has an equivalent in Jest/Jasmine. You can do this:

it('does something', () => {
  // some assertion
}, 500);

Which sets a 500ms timeout. I don't think it's available for describe blocks though, so if you wanted to add support for this it'd need to be detected on the suite and then added to each it.

[AlanFoster]

Thanks for providing an awesome codeshift for this migration 👍

For visibility I ran this locally, and ran into some issues.

This was my before code:

const asyncCall = function () {
  return Promise.resolve({ foo: 10 });
};

describe('async example', function () {
  beforeEach(async function () {
    this.response = await asyncCall();
  });

  it('makes the call with token', async function () {
    expect(this.response).toEqual({ foo: 10 });
  });
});

And this was my after code:

const asyncCall = function () {
  return Promise.resolve({ foo: 10 });
};

describe('async example', () => {
  let responseContext;
  beforeEach(() => {
    responseContext = await asyncCall();
  });

  it('makes the call with token', () => {
    expect(responseContext).toEqual({ foo: 10 });
  });
});

The after code no longer compiles:

  ● Test suite failed to run

     await is a reserved word (8:22)
         6 |   let responseContext;
         7 |   beforeEach(() => {
      >  8 |     responseContext = await asyncCall();
           |                       ^
         9 |   });
        10 |
        11 |   it('makes the call with token', () => {

i.e. The change from async function to using a fat arrow syntax means the await keyword is no longer valid.

It's possible to fix this by manually adding the async keyword:

  beforeEach(async () => {
    responseContext = await asyncCall();
  });

This may not be a problem with this particular code shift, but I thought i'd flag it up for visibility 👍

[avaly]

@AlanFoster Thanks for reporting the issue. Indeed I overlooked async functions. I'll patch the codemod when I get a chance to treat async functions as well.

[AlanFoster]

@avaly No problem! Thanks for the code shift, it saved us a bunch of time 👍

Out of interest - what are your thoughts on this approach:

Extract this variables to reference a context variable at the top, which can be cleared before every test.

So for one of your examples, we introduce a context variable at the top - which is accessible in the beforeEach and tests.

Current approach:

describe('foo', () => {
    let barContext;
    let fooContext;
    beforeEach(() => {
        fooContext = { id: 'FOO' };
        barContext = { id: 'BAR', child: fooContext };
    });

    it('should have proper IDs', () => {
        expect(fooContext.id).to.equal('FOO');
        expect(barContext.id).to.equal('BAR');
        expect(barContext.child.id).to.equal('FOO');
    });
});

Shared context approach:

describe('foo', () => {
    let context;
    beforeEach(() => {
      context = {};
    });    
   
    beforeEach(() => {
        context.foo = { id: 'FOO' };
        context.bar = { id: 'BAR', child: context.foo };
    });

    it('should have proper IDs', () => {
        expect(context.foo.id).to.equal('FOO');
        expect(context.bar.id).to.equal('BAR');
        expect(context.bar.child.id).to.equal('FOO');
    });
});

It stops the scenario of leaking test state between tests without realizing it, or tests passing by mistake. It also ensures that future developers who modify the codeshifted files continue to make use of the naming convention of context to represent test state - as I know in the future other developers may forget the Context suffix unless we added an explicit linting rule for that

[skovhus]

Thanks for the feedback @AlanFoster. And thanks @avaly for having a look at fixing it. 🤘🏻

[avaly]

@AlanFoster I've added the async function support. Feel free to give it another try.

I like your suggestion with the context object. I will give that a try later this week. Does anyone else have any better names for context?

[avaly]

I've updated the codemod using @AlanFoster's suggestion in #61 (comment) of grouping all context data in a shared object.

[AlanFoster]

@avaly The latest code shift works great! I was able to transform the entire codebase without any issues, and all tests pass :)

One thing that is catching my eye is the rewrite of function () { to ArrowFunctionExpressions within our codebase:

-    it('sets initial state', function() {
-      expect(this.state).toEqual({});
+    it('sets initial state', () => {
+      expect(testContext.state).toEqual({});
     });

This might be a jest best practice though? I haven't tried out any of the other jest-codemods, but I wasn't able to see any linting rules to enforce using fat arrows within description blocks within eslint-plugin-jest for instance

Let me know your thoughts if this code shift should preserve the original function type or always align with ArrowFunctionExpressions 👍

[skovhus]

@AlanFoster thanks for testing it.

I think using arrow functions makes sense for most people and is supported from node@4...

Back from vacation now, so let us get this released! @avaly thank you so much for your work.

[avaly]

@AlanFoster The main reason I chose to also replace normal functions with arrow functions is that since we no longer use this for context, this will point to some upper scope global inside arrow functions, thus making any this usage incorrect.