added tests #5

Inve1951 · 2018-08-11T16:52:04Z

added a couple tests
i'm aware not all in there is ideal but please point me in the right direction

i thought to maybe move hasActualAllExpectedPropsAndAreTheyEqual to the top, ~~give it array support~~ (already has), and use it for all the tests

This reverts commit 8125718.

GeoffreyBooth · 2018-08-11T17:29:23Z

Could hasActualAllExpectedPropsAndAreTheyEqual be replaced by Node's assert.deepStrictEqual?

Inve1951 · 2018-08-11T17:50:37Z

Could hasActualAllExpectedPropsAndAreTheyEqual be replaced by Node’s assert.deepStrictEqual?

no, they work differently:

actual = a: 1, b: 2
expected = a: 1   # don't care about `b`, ignore it

hasActualAllExpectedPropsAndAreTheyEqual actual, expected
# undefined

deepStrictEqual actual, expected   # throws
# AssertionError: { a: 1, b: 2 } deepStrictEqual { a: 1 }

deepStrictEqual even compares prototypes and symbols etc whereas hasActualAllExpectedPropsAndAreTheyEqual only checks enumerable keys of expected.

do those tests look alright so far? i got a feeling that i’m doing something awfully wrong.
also i couldn’t find some nodes, e.g. Extends. do they not exist?

new helpers for cleaner code

tests are mostly complete now. also included one for our helper function.

Inve1951 · 2018-08-13T07:15:04Z

@GeoffreyBooth I believe this is ready for a review.

GeoffreyBooth

Great work so far! All minor notes.

GeoffreyBooth · 2018-08-13T15:21:21Z

test/abstract_syntax_tree.coffee

+
+testExpressions = (code, expected) ->
+  ast = getExpressions code
+  return console.log ast unless expected?


I think I added these console.log lines as helpers for writing the tests, so I could figure out what expected should be. Now that all the tests are written we don’t need them anymore.

I added those logs and would leave them there for future edits. They are very convenient.

GeoffreyBooth · 2018-08-13T15:21:33Z

test/abstract_syntax_tree.coffee

+
+testExpression = (code, expected) ->
+  ast = getExpression code
+  return console.log ast unless expected?


Here’s the other one.

GeoffreyBooth · 2018-08-13T15:22:28Z

test/abstract_syntax_tree.coffee

+    x: [3, 2, 1]
+
+  check 'Partial object comparison', doesNotThrow,
+    a: c: 2


This line looks confusing. Is it:

a: c: 2

?

Yes it is. To me those aren't confusing at all. It's just coffeescript.

GeoffreyBooth · 2018-08-13T15:25:57Z

test/abstract_syntax_tree.coffee

+    body:
+      type: 'Value'
+      isDefaultValue: no
+      base: value: 'i'


In general can we avoid implicit objects as object values? It seems like a recipe for trouble later.

base: value: 'i'

Well, we could. But not only would it make the file much longer, it would also make it less readable IMO.

Unless I’m mistaken, it’s not the coding style of the rest of the codebase. We try to keep things consistent.

GeoffreyBooth · 2018-08-13T15:28:11Z

test/abstract_syntax_tree.coffee

+  testExpression '///^#{flavor}script$///',
+    type: 'RegexWithInterpolations'
+
+  # TODO: Shouldn't there be more info we can check for?


Could you check for the body of the regex?

That's what i wanted to do but it seems the information is missing. Here's the full AST from getExpressions:

[ { type: 'RegexWithInterpolations', loc: { start: {line: 0, column: 0}, end: {line: 0, column: 22} } } ]

Yeah, I think this is the kind of thing that @helixbass is adding in his work.

GeoffreyBooth · 2018-08-13T15:31:51Z

test/abstract_syntax_tree.coffee

+    clause: type: 'Class'

 test "AST as expected for ExportAllDeclaration node", ->
  # TODO


We could add this one, couldn’t we?

Unfortunately I have no idea what it could be.

export * from "module-name"

GeoffreyBooth · 2018-08-13T15:32:21Z

test/abstract_syntax_tree.coffee

  # TODO

 test "AST as expected for ModuleSpecifierList node", ->
  # TODO


I think ModuleSpecifierList is a parent class that would never be output on its own. If that’s correct, we shouldn’t need to test it (well, we couldn’t, actually).

I was under the same impression. It's likely the base class for ExportSpecifierList and ImportSpecifierList. Same would go for the other Module nodes.

If it’s never output in the AST, we don’t need a TODO for it.

GeoffreyBooth · 2018-08-13T15:33:35Z

test/abstract_syntax_tree.coffee

+  # TODO: Test object splats.

 test "AST as expected for Expansion node", ->
  # TODO


Can we add this one, and the following?

No idea what they could be.

Expansion is defined in a comment as “Used to skip values inside an array destructuring (pattern matching) or parameter list.” I wonder if this node is even used anymore, now that we use ES destructuring.

Elision is just [,a, , , b, , c, ,] (the skipped elements). Is that not output in the AST?

GeoffreyBooth · 2018-08-13T15:35:08Z

test/abstract_syntax_tree.coffee

+          base: value: 'o'
+      ,
+        originalValue: '/'
+      ]


For strings, we will want to test (eventually) that the AST outputs the string delimiter (single quote or double quote). I assume that’s not output yet, so maybe add a TODO?

That information is available in the quote property and is even checked here. I added the check to StringLiteral now.

Ah, sorry I didn’t notice. Good catch.

GeoffreyBooth · 2018-08-13T15:35:39Z

test/abstract_syntax_tree.coffee

+      base: value: '42'
+      isDefaultValue: no
+
+  # TODO: File issue for compile error when using `then` or `;` where `\n` is rn.


From playing around for a bit it seemed impossible to enter the Switch body without an actual linebreak. From my understanding a semicolon or the then keyword should allow for it.

GeoffreyBooth · 2018-08-13T15:39:15Z

do those tests look alright so far? i got a feeling that i’m doing something awfully wrong.
also i couldn’t find some nodes, e.g. Extends. do they not exist?

I think you’re on the right track. In general we probably want to check more values of things, like the value of a string or the name of a function/Call. We especially want to check the unusual attributes of nodes, like soak etc., to make sure that they exist and are what we’d expect.

As for Extends, it may be that it’s just never output in the final AST because it becomes included in some larger node—in that case, probably a Class. If that’s the case, we should try to find it inside that larger node and validate it there.

Inve1951 · 2018-08-13T16:57:32Z

Replied to your comments.

I think you’re on the right track. In general we probably want to check more values of things, like the value of a string or the name of a function/Call. We especially want to check the unusual attributes of nodes, like soak etc., to make sure that they exist and are what we’d expect.

Absolutely. But I feel that the current AST generator isn't quite there yet. Some information seems missing (RegexWithInterpolations) or does not keep track of the originally used code (unless). And some I can't make sense of (return prop on For nodes).

As for Extends, it may be that it’s just never output in the final AST because it becomes included in some larger node—in that case, probably a Class. If that’s the case, we should try to find it inside that larger node and validate it there.

It seems that Extends does not have it's own node. However extending a class leaves the Class node with a parent property. That's what the test currently checks for.

If i followed correctly then someone (@helixbass?) is still working on AST output. The tests I wrote in this PR so far all pass on the current compiler output, which i expect will still change as work on the AST generator progresses. I'd rather not make the tests too strict just yet before the AST is final.

helixbass · 2018-08-14T13:31:14Z

@Inve1951 yup I have been working on AST generation on a separate branch

Wherever possible, I've been making the structure of our AST nodes match the corresponding Babel AST nodes, as there are lots of upsides to compatibility. Then when there are Coffeescript-specific things, I've exposed additional fields on some node types and created a couple new node types that don't have a comparable node type in JS

@GeoffreyBooth and I discussed a strategy for getting the work I've been doing merged into our ast branch where I can open individual PRs for each node type's AST support and make sure that the corresponding AST tests reflect the expected shape of the AST nodes (including any edge cases)

So basically I'll be prepared to pick up the existing tests on this branch in whatever state they're in and tweak them/add to them as I go

If you're curious, I'd recommend astexplorer.net as a great way of exploring the different AST node types - choosing the babylon7 parser should allow you to inspect the Babel-style AST nodes for any ES6+ code. And/or you can check out my branch and try dumping ASTs for Coffeescript source using the --ast command-line flag (let me know if you need help getting this working)

Inve1951 · 2018-08-14T15:25:10Z

@helixbass i cloned your prettier branch to see how much the AST differs. However it fails to build the compiler as it can't handle the object splats. I've tried installing babel-plugin-transform-object-rest-spread, adding it to .babelrc and even to coffeescript.coffee's compile function but it still fails to cake build:full when encountering a splat. No idea how to resolve this (besides upgrading node).

helixbass · 2018-08-14T15:38:32Z

@Inve1951 ah yeah I recall the discussion that there shouldn't be object spread syntax in the Coffeescript codebase so as to still support Node 6.x. So I'll have to go through and update places where I'm using it

In the meantime, if upgrading Node isn't an option, I'm not sure exactly how you'd have to configure Babel to get it to post-process the compiled lib/coffeescript/*.js files. If you want to get really dirty, you could probably hack it into the build process manually eg by adding a Babel invocation at the end of buildExceptParser() in Cakefile

…und babel/minify#893 (jashkenas#5095)

GeoffreyBooth · 2018-08-14T15:50:13Z

Please merge master into your branches, I added a fix for dependencies.

@helixbass I’ve been using Object.assign instead of object splats, if you look around the codebase you can find some examples.

Building the compiler doesn’t use Babel, other than for building the browser compiler. On master it builds from Node 4 and up. Object splats are supported in Node 8 and up.

Inve1951 · 2018-08-16T14:56:27Z

Updated per your comments. These node types are still left without any tests: ImportNamespaceSpecifier, Expansion

…ests

…d, built files in /lib; the build parts of CI are only a test that the build process succeeds, so we shouldn't be testing the output of the CI build steps

Paul Nta and others added 3 commits June 16, 2018 15:32

fix momentum scrolling on iOS (jashkenas#5083)

8125718

Revert "fix momentum scrolling on iOS (jashkenas#5083)" (jashkenas#5084)

7f8b36a

This reverts commit 8125718.

added tests

d0b40fc

Inve1951 added 2 commits August 12, 2018 20:17

cleanup

90d1517

new helpers for cleaner code

more tests

3922c95

tests are mostly complete now. also included one for our helper function.

GeoffreyBooth requested changes Aug 13, 2018

View reviewed changes

string quotes

fbe0bd8

Update dependencies; disable some babel-minify transforms to work aro…

98b2a69

…und babel/minify#893 (jashkenas#5095)

Inve1951 and others added 4 commits August 16, 2018 16:57

more tests + refactor

c6e8577

Remove unused helper function; improve logging output

e65d530

Add tests for ImportNamespaceSpecifier, Expansion; cleanup

4a01b7a

Merge branch 'master' of github.com:jashkenas/coffeescript into ast-t…

f0ffc1a

…ests

GeoffreyBooth approved these changes Aug 18, 2018

View reviewed changes

CI: Test first before building, so that the tests run on the committe…

28810be

…d, built files in /lib; the build parts of CI are only a test that the build process succeeds, so we shouldn't be testing the output of the CI build steps

GeoffreyBooth merged commit 13ba458 into GeoffreyBooth:ast-tests Aug 18, 2018

added tests #5

added tests #5

Uh oh!

Conversation

Inve1951 commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoffreyBooth commented Aug 11, 2018

Uh oh!

Inve1951 commented Aug 11, 2018 • edited by GeoffreyBooth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Inve1951 commented Aug 13, 2018

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Aug 13, 2018

Uh oh!

Inve1951 commented Aug 13, 2018

Uh oh!

helixbass commented Aug 14, 2018

Uh oh!

Inve1951 commented Aug 14, 2018

Uh oh!

helixbass commented Aug 14, 2018

Uh oh!

GeoffreyBooth commented Aug 14, 2018

Inve1951 commented Aug 11, 2018 •

edited

Loading

Inve1951 commented Aug 11, 2018 •

edited by GeoffreyBooth

Loading