Add a codemod

[simonihmig]

Has anybody thought/worked on a codemod already to automatically refactor jQuery based tests?

Would like to get rid off jQuery from ember-bootstrap (@cibernox thanks for the awesome blog post btw!), but refactoring the test suite manually is probably the most work.

Certainly it would be next to impossible to cover all possible cases of chainable jQuery code, but at least for the 90% of very common cases (assertions based on .length, .text(), .hasClass()) there could be some solution that relieves us at least from most of the work.

Have not worked with AST before, but would be a good use case to learn something new, unless there is already some work going into that direction?

[cibernox]

I think that creating a codemod that is midly useful and doesn't break things would be a lot of work, if possible.

There is a lot of patterns that are extremely hard to know what the user wanted to do. By example:

assert.equal($('.some-list-item').text(), 'foo bar baz');

What was the intention of the user above? Asserting that there is one element like <li class="some-list-item">Foo bar baz</li> or asserting that there is three elements? <li class="some-list-item">foo</li><li class="some-list-item">bar</li><li class="some-list-item">baz</li>. We can't know.

Also, there is some patterns that if translated 1:1 are a bit unidiomatic now.

Think about this example:

assert.equal(find('.warning-msg').length, 1, 'There is one warning message');

If the user wants to express that some element is present, this would be more idiomatic like

assert.ok(find('.warning-msg'), 'There is one warning message');

If on the contrary the user wants to assert that there is one but no more than one warning messages, the idiomatic approach would be

assert.equal(findAll('.warning-msg').length, 1, 'There is only one warning message');

This shade on the meaning didn't exist before because there was no distinction between find and findAll, but now there is and tests can tell the store a bit better.

I think that creating a codemod that is "not crap" would be pretty hard. It's ok for codemods to not catch everything, but it's not ok to perform bad transformations. I'd rather have a codemod that does only 10% of the things but does them right than one that does 50% of the things but makes mistakes a 5% of the time.

Perhaps there is some small subset of changes a codemod can safely do, idk.

[simonihmig]

I understand your concerns, for sure. But at least when looking at my codebase, the three cases of assertions I mentioned cover at least 90%, so there is some real value in handling them automatically.

For the non-ideomatic case: I would prefer matching the exact semantics of the assertion (which would result in the findAll().length type of assertion). You could have written the jQuery based one with assert.ok(find('.warning-msg').length) as well to not care about the exact number, so I think we should not change semantics here.

For your .text() example, I hope nobody does something like this, but who knows!? 😬

What about just creating a collection of micro-transforms, so everybody can pick and choose what transform makes sense for his/her case? As I said, for me the mentioned three transforms would cover most of the work, and if it's documented what they do, they should not do any harm to my codebase...

I will probably try to play with those that I would need now, and then we can see if it makes sense to make them available for the general public. Does that make sense to you?

[cibernox]

Sure, play with it.

The general rules are:

Acceptance:

1. Convert click() to await click() (same with fillIn and others)
2. So step 1 only if the selector inside is a valid CSS3 selector (no :contains/:eq/:odd)
3. At the end of a test check what helpers were used and add the proper import statement.
4. If a test had one await, add async to the function.
5. Convert this.$(selector).text() to find(selector).textContent (and assume nobody does the thing I mentioned)
6. Convert this.$(selector).length to findAll(selector).length
7. Convert this.$(selector).click() => click(selector);
8. Convert this.$(selector).focus() => focus(selector);
9. Convert this.$(selector).val('something') => fillIn(selector);
10. Convert this.$(selector).val() => find(selector).value;
11. Convert this.$(selector).trigger('keydown', { keyCode: num }) => keyEvent(selector, 'keydown', num);
12. Very likely, convert $el.attr('id') to el.id

This alone is like 70% of the work.

Given that the change of getting things wrong along the way ideally the user should be propted to review and answer Y/N on every step.

[simonihmig]

I put something together, you might wanna have a look: https://github.com/simonihmig/ember-native-dom-helpers-codemod

Still a WIP, but proved quite useful so far. Includes the above transformations and a couple more, so far just for integration tests (my primary use case). Of course, depending on your test code style, not able to cover everything, but given the quite big test suite of ember-bootstrap was able to transform around maybe 80%, without messing things up (too much 😬)...

[cibernox]

This is fantastic. I'm on holidays until Wednesday but I think you're saving tens of man-hours to the community.

[simonihmig]

@cibernox Thanks! I have seen the issue you created, there is certainly room for some improvements. Enjoy your holiday for now, and looking forward to your feedback! :)

[rwjblue]

@simonihmig / @cibernox - Seems like this codemod is working well for folks, should we close this issue? Are there any remaining tasks here?

[cibernox]

Yes. I added a link in the readme already. Thanks @simonihmig!

[simonihmig]

Sure, this can be closed. Forgot about it...