ember-codemods / ember-mocha-codemods

Codemod scripts for ember-mocha
MIT License
4 stars 6 forks source link

render calls in before/beforeEach aren't converted #32

Closed apellerano-pw closed 6 years ago

apellerano-pw commented 6 years ago

Sometimes when tests have similar setups we use a beforeEach to call render. The codemod does a good job converting it usages to async functions but misses functions passed to other mocha hooks

ghost commented 6 years ago

This sounds like the same issue in: https://github.com/Turbo87/ember-mocha-codemods/pull/26. would you mind confirming? I'm picking the codemod up again today to see if I can get it rewriting properly.

apellerano-pw commented 6 years ago

Yes it seems like the same issue to me. Thanks for making a fix!

ghost commented 6 years ago

cool thanks! no problem. I think I may have fixed the issue here: https://github.com/efx/ember-mocha-codemods/tree/fix-14 If you have time, you could try that branch against your codebase and let me know if you have any issues.

apellerano-pw commented 6 years ago

I ran it on a file, here is the before and after.

While it did convert the render to the one imported from @ember/test-helpers, it didn't add async or await to the code so I don't think it would be a sufficient fix for my case

beforeEach(function () {
  this.render(hbs`{{my-component
    isActiveCollection=true
    collection=collection
  }}`);
});
beforeEach(function () {
  render(hbs`{{my-component
    isActiveCollection=true
    collection=collection
  }}`);
});
apellerano-pw commented 6 years ago

Here's an input file

import { expect } from 'chai';
import {
  beforeEach,
  context,
  describe,
  it,
} from 'mocha';
import { setupComponentTest } from 'ember-mocha';
import hbs from 'htmlbars-inline-precompile';

describe('Integration | Component | my-component', function () {
  setupComponentTest('my-component', {
    integration: true,
  });

  context('when in some situation or another', function () {
    beforeEach(function () {
      this.render(hbs`{{my-component}}`);
    });

    it('should do the thing', function () {
      expect(true).to.be.true;
    });
  });
});

I ran it thru your fork with jscodeshift -t https://raw.githubusercontent.com/efx/ember-mocha-codemods/fix-14/fourteen-testing-api.js fake-test.js

ghost commented 6 years ago

sweet, thanks for checking. I have found a few other bugs too. I will add this snippet above as a test too.

ghost commented 6 years ago

@apellerano-pw I just updated the branch with a passing test accounting for the context hook. If it runs ok on your codebase, I'll open the PR.

apellerano-pw commented 6 years ago

ayyyy it worked! :tada:

apellerano-pw commented 6 years ago

Oh, if the problem was that the context() alias wasn't working, what about the specify() alias for it()? I didn't see that in a quick search of the codemod either. This might be worthy of its own ticket

https://mochajs.org/#bdd

context() is just an alias for describe(), and behaves the same way; it just provides a way to keep tests easier to read and organized. Similarly, specify() is an alias for it().

ghost commented 6 years ago

good point. I think a new ticket is best so we can keep the PR's smaller.

apellerano-pw commented 6 years ago

I made #37 for it