Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

Chore: AST alignment testing against Babylon #342

Merged
merged 2 commits into from
Aug 7, 2017
Merged

Conversation

JamesHenry
Copy link
Member

@JamesHenry JamesHenry commented Jul 21, 2017

@soda0289 @Andy-MS

Now that the TypeScript PR has been merged into Babylon, and the effort to align on a TypeScript AST has become somewhat "official", I would like to propose that we begin formally testing AST alignment on our existing codebase.

As you know I had set up https://github.com/JamesHenry/tsep-babylon-test for this purpose, but it is already incredibly cumbersome to have to swich contexts and repos to try out ideas and make improvements.

This PR shows how testing for AST alignment could work.

I have added a new section to the tests/ directory, with a dedicated spec runner (still using jest).

To run the tests, for now, you just do:

npm run ast-alignment-tests

I have not yet made them a required part of the primary testing for the repo, but we may trend towards that.

As you can see in the diff, I was able to get "some" tests to pass straight out of the gate:

"basics/update-expression.src.js",
"basics/new-without-parens.src.js",
"ecma-features/arrowFunctions/**/as*.src.js"

This does not truly reflect the current state as I did not bother to comb through all the subdirectories to find working cases, because the vast majority still seem to be failing. In some cases this is down to genuine differences in TypeScript ASTs, but in many it is down to very tiny differences in LOC data.

E.g. If there is a newline at the end of the file, tsep and babylon seem to disagree on LOC end.

Another random thing I noticed - tsep is adding start: Number and end: Number for JSX code, but we do not do that for standard code. I have set up the repo to strip the start and end number values from babylon ASTs to account for the predominant behaviour, but it is weird that we have this inconsistency within tsep. I have created an issue for that here: #341

Let me know if you guys have any feedback, I am feeling quite good about this setup and I think it will allow us to converge in the quickest and easiest possible way.

@eslintbot
Copy link

Thanks for the pull request, @JamesHenry! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@JamesHenry JamesHenry changed the title WIP Chore: AST alignment testing against Babylon Jul 21, 2017
@eslintbot
Copy link

Thanks for the pull request, @JamesHenry! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@JamesHenry JamesHenry requested a review from a team August 7, 2017 16:26
@eslintbot
Copy link

Thanks for the pull request, @JamesHenry! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@JamesHenry JamesHenry merged commit 5e22fac into master Aug 7, 2017
@JamesHenry JamesHenry deleted the jh-babylon-tsep branch August 7, 2017 20:49
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants