feat(ast-spec): add fixture test framework and some initial fixtures #3258

bradzacher · 2021-04-01T22:28:32Z

Add fixture tests for each and every AST node.
To make things more readable I:

separated the AST from the tokens
- this stops the individual snapshot from being so long it's impossible to understand
- if there are changes to tokens - then they should be more obvious now. Before you had to expand the snapshot in github to understand that the token was part of the tokens array.
always write error snapshot, and always write AST/token snapshots
- This is just for consistency and to make it easier whilst you're developing.
- It will prevent us from accidentally leaving behind error snapshots if they weren't supposed to be there.
added a custom snapshot serializer
- Having learned how useful they are with scope manager - I created a new one to improve the look of the snapshots.
- Instead of sorting alphabetically, I place type at the top, and range/loc at the end.
- Instead of outputting Object ahead of every node, instead it outputs the node type.
- I adjusted the output range/loc so they take up fewer lines and are more compact.

I prefixed the snapshot names with numbers just so we can control the sorting of them. No other reason.

Work In Progress

I wanted to get some early feedback on things like the folder structure and the snapshot representation before I go too far down this rabbit hole.

TODO:

Add fixtures for every single node
Add babel parse snapshots
Add babel alignment snapshots

typescript-eslint · 2021-04-01T22:28:36Z

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

bradzacher · 2021-05-30T22:36:30Z

.../src/declaration/ClassDeclaration/fixtures/implementsMany/snapshots/7-AST-Alignment-AST.shot

+
+          range: [46, 48],
+          loc: {
+            start: { column: 46, line: 1 },
+            end: { column: 48, line: 1 },
+          },
+        },
+        id: Identifier {
+          type: 'Identifier',
+          name: 'Foo',
+
+          range: [6, 9],
+          loc: {
+            start: { column: 6, line: 1 },
+            end: { column: 9, line: 1 },
+          },
+        },
+        implements: Array [
+-         TSClassImplements {
+-           type: 'TSClassImplements',
+         TSExpressionWithTypeArguments {
+           type: 'TSExpressionWithTypeArguments',
+            expression: Identifier {
+              type: 'Identifier',
+              name: 'Object',
+
+              range: [21, 27],
+              loc: {
+                start: { column: 21, line: 1 },
+                end: { column: 27, line: 1 },
+              },
+            },
+
+            range: [21, 27],
+            loc: {
+              start: { column: 21, line: 1 },
+              end: { column: 27, line: 1 },
+            },
+          },
+-         TSClassImplements {
+-           type: 'TSClassImplements',
+         TSExpressionWithTypeArguments {
+           type: 'TSExpressionWithTypeArguments',
+            expression: Identifier {
+              type: 'Identifier',
+              name: 'Function',
+
+              range: [29, 37],
+              loc: {
+                start: { column: 29, line: 1 },
+                end: { column: 37, line: 1 },
+              },
+            },
+
+            range: [29, 37],
+            loc: {
+              start: { column: 29, line: 1 },
+              end: { column: 37, line: 1 },
+            },
+          },
+-         TSClassImplements {
+-           type: 'TSClassImplements',
+         TSExpressionWithTypeArguments {
+           type: 'TSExpressionWithTypeArguments',
+            expression: Identifier {
+              type: 'Identifier',
+              name: 'RegExp',
+
+              range: [39, 45],
+              loc: {
+                start: { column: 39, line: 1 },
+                end: { column: 45, line: 1 },
+              },
+            },
+
+            range: [39, 45],
+            loc: {
+              start: { column: 39, line: 1 },
+              end: { column: 45, line: 1 },
+            },
+          },
+        ],
+        superClass: null,
+
+        range: [0, 48],
+        loc: {
+          start: { column: 0, line: 1 },
+          end: { column: 48, line: 1 },
+        },
+      },
+    ],
+    sourceType: 'script',
+
+    range: [0, 49],
+    loc: {
+      start: { column: 0, line: 1 },
+      end: { column: 0, line: 2 },
+    },
+  }"
+`;


example of an AST misalignment.

Rendered in GH

bradzacher · 2021-05-30T22:37:30Z

...c/declaration/ClassDeclaration/fixtures/implementsMany/snapshots/8-AST-Alignment-Tokens.shot

+      range: [0, 5],
+      loc: {
+        start: { column: 0, line: 1 },
+        end: { column: 5, line: 1 },
+      },
+    },
+    Identifier {
+      type: 'Identifier',
+      value: 'Foo',
+
+      range: [6, 9],
+      loc: {
+        start: { column: 6, line: 1 },
+        end: { column: 9, line: 1 },
+      },
+    },
+-   Keyword {
+-     type: 'Keyword',
+   Identifier {
+     type: 'Identifier',
+      value: 'implements',
+
+      range: [10, 20],
+      loc: {
+        start: { column: 10, line: 1 },
+        end: { column: 20, line: 1 },
+      },
+    },
+    Identifier {
+      type: 'Identifier',
+      value: 'Object',
+
+      range: [21, 27],
+      loc: {
+        start: { column: 21, line: 1 },
+        end: { column: 27, line: 1 },
+      },
+    },
+    Punctuator {
+      type: 'Punctuator',
+      value: ',',
+
+      range: [27, 28],
+      loc: {
+        start: { column: 27, line: 1 },
+        end: { column: 28, line: 1 },
+      },
+    },
+    Identifier {
+      type: 'Identifier',
+      value: 'Function',
+
+      range: [29, 37],
+      loc: {
+        start: { column: 29, line: 1 },
+        end: { column: 37, line: 1 },
+      },
+    },
+    Punctuator {
+      type: 'Punctuator',
+      value: ',',
+
+      range: [37, 38],
+      loc: {
+        start: { column: 37, line: 1 },
+        end: { column: 38, line: 1 },
+      },
+    },
+    Identifier {
+      type: 'Identifier',
+      value: 'RegExp',
+
+      range: [39, 45],
+      loc: {
+        start: { column: 39, line: 1 },
+        end: { column: 45, line: 1 },
+      },
+    },
+    Punctuator {
+      type: 'Punctuator',
+      value: '{',
+
+      range: [46, 47],
+      loc: {
+        start: { column: 46, line: 1 },
+        end: { column: 47, line: 1 },
+      },
+    },
+    Punctuator {
+      type: 'Punctuator',
+      value: '}',
+
+      range: [47, 48],
+      loc: {
+        start: { column: 47, line: 1 },
+        end: { column: 48, line: 1 },
+      },
+    },
+  ]"
+`;


example of an token misalignment.

Rendered in GH

bradzacher · 2021-05-30T22:39:51Z

packages/ast-spec/tests/fixtures-with-differences-ast.shot

+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`AST Fixtures List fixtures with AST differences 1`] = `
+Set {
+  "src/declaration/ClassDeclaration/fixtures/implementsMany/fixture.ts",
+  "src/declaration/ClassDeclaration/fixtures/implementsOne/fixture.ts",
+  "src/declaration/ClassDeclaration/fixtures/typeParameters/fixture.ts",
+  "src/declaration/ClassDeclaration/fixtures/typeParametersExtendsTypeParameters/fixture.ts",
+}
+`;


snapshot which provides a centralised list of fixtures with AST differences

bradzacher · 2021-05-30T22:39:58Z

packages/ast-spec/tests/fixtures-with-differences-tokens.shot

+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`AST Fixtures List fixtures with Token differences 1`] = `
+Set {
+  "src/declaration/ClassDeclaration/fixtures/implementsMany/fixture.ts",
+  "src/declaration/ClassDeclaration/fixtures/implementsOne/fixture.ts",
+}
+`;


snapshot which provides a centralised list of fixtures with Token differences

nx-cloud · 2021-09-05T20:28:52Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit fb13fb5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 43 targets

Sent with 💌 from NxCloud.

bradzacher · 2021-09-10T06:44:04Z

packages/ast-spec/tests/fixtures-with-differences-errors.shot

+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`AST Fixtures List fixtures with Error differences 1`] = `
+Object {
+  "Babel errored but TSESTree didn't": Set {
+    "declaration/ClassDeclaration/fixtures/_error_/implements-non-identifier/fixture.ts",
+    "declaration/ExportAllDeclaration/fixtures/_error_/non-string-source/fixture.ts",
+    "declaration/ExportAllDeclaration/fixtures/_error_/type-kind/fixture.ts",
+    "declaration/ExportNamedDeclaration/fixtures/_error_/anonymous-class/fixture.ts",
+  },
+  "TSESTree errored but Babel didn't": Set {
+    "declaration/ExportAllDeclaration/fixtures/_error_/named-non-identifier/fixture.ts",
+    "declaration/ExportNamedDeclaration/fixtures/_error_/aliased-literal/fixture.ts",
+  },
+}
+`;


snapshot which provides a centralised list of fixtures with error differences (not error messages, just existence of errors)

bradzacher · 2021-10-29T03:24:25Z

@JoshuaKGoldberg - wanna take a look? Got any opinions about this broken-up style of things vs the gigantic bag of fixtures that we have currently?

JoshuaKGoldberg · 2021-10-30T15:58:52Z

Sure! I'll try to find time Soon™️.

bradzacher · 2022-04-22T02:04:39Z

The core of this is done - all that's left is creating fixtures for each and every case.
Instead of holding it in draft until I find the time to do this - I'm going to put this up for review so we can merge it.

That way we can make all new AST changes add fixtures here, and migrate the old fixtures piece-by-piece - we can distribute the work so I'm not the blocker.

packages/ast-spec/tests/fixtures.test.ts

JoshuaKGoldberg

🔥. Just a few small code nitpicks from me on re-review, nothing close to blocking. Awesome!!

I'd want to hear from @JamesHenry on the Nx shenanigans, if James has time.

packages/ast-spec/tests/util/parsers/babel.ts

JoshuaKGoldberg · 2022-04-22T17:32:38Z

packages/ast-spec/tests/util/serializers/string.ts

+    characters.push("'");
+
+    return characters.join('');
+  },


This functionally behaves about the same as str::replaceAll("'", '\\'), no?

I think you could technically write this as

str.replaceAll("\\", "\\\\").replaceAll("'", "\\'");

Though that would be slower because you'd need to do it in two passes over the string.

I haven't perf tested it, but given that there are going to be hundreds of these strings - each with a few hundred characters, I think that double handling will add up.

JoshuaKGoldberg · 2022-04-22T17:34:36Z

packages/ast-spec/tests/util/typescript-estree-import.ts

+ * - Removing @typescript-eslint/typescript-estree as an explicit devDependency in the package.json
+ * - Add an `.nxignore` file at the root of the monorepo which ignores this file (which we then in turn
+ * ensure is the only place we directly import from the @typescript-eslint/typescript-estree package).
+ */


This feels like something we (@JamesHenry 😄) should feature request into Nx: the ability to have technical circular dependencies only in tests...

I can't remember the exact history of this but I'm pretty sure I provided this breakdown and created this solution to the problem.

As it turns out on this PR right now the @typescript-eslint/typescript-estree package is not in the package.json of ast-spec, so we could simplify things to:

The specifying of the package in the package.json was the real issue, and is the thing that causes the rigid circular dependency to exist between the nodes on the graph - there would be no way to know that the package was only used in certain situations when that is the way the relationship is defined. As part of the Nx updates I'm doing we'll be moving entirely to source to source relationships between packages/graph nodes (as is far more common) and we won't need to worry about this in that situation

(I didn't include it in the screenshot but feel free to move some of the commentary around why we don't want to include the package in the package.json to make that lint rule happy and that's why we are disabling it inline)

Was there an update to nx to allow this? I swear there was a reason we went with using an .nxignore file.

Or did we used to rely on implicit dependency graph and now we have an explicit one configured it doesn't matter?

I'm afraid I don't 100% remember 😬

packages/ast-spec/typings/babel-eslint-parser.d.ts

yarn.lock

packages/types/tools/copy-ast-spec.ts

JamesHenry · 2022-04-23T12:49:15Z

@JoshuaKGoldberg @bradzacher I’ve set aside some time tomorrow 👍

packages/ast-spec/src/declaration/ClassDeclaration/fixtures/extends-literal/README.md

packages/ast-spec/tests/util/serializers/Node.ts

packages/ast-spec/tests/util/snapshot-diff.ts

bradzacher · 2022-04-24T18:03:38Z

addressed comments.

side note: I was thinking that this could be a platform for us to work on #1852.
we could use the fixtures to generate an "expected" AST shape and compare it to our types to determine where the types don't match the values.

we also could be running alignment tests against espree to help us identify and track where we diverge from the core ESTree spec

JamesHenry · 2022-04-24T18:08:23Z

Great work, @bradzacher!

codecov · 2022-04-24T18:30:01Z

Codecov Report

Merging #3258 (fb13fb5) into main (88ed9ec) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3258   +/-   ##
=======================================
  Coverage   94.23%   94.24%           
=======================================
  Files         152      152           
  Lines        8281     8283    +2     
  Branches     2694     2696    +2     
=======================================
+ Hits         7804     7806    +2     
  Misses        263      263           
  Partials      214      214

Flag	Coverage Δ
unittest	`94.24% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/no-namespace.ts	`100.00% <0.00%> (ø)`
...ackages/eslint-plugin/src/rules/space-infix-ops.ts	`100.00% <0.00%> (ø)`
...ges/eslint-plugin/src/rules/no-misused-promises.ts	`98.25% <0.00%> (ø)`

Add fixture tests for each and every AST node. To make things more readable I: - separated the AST from the tokens - this stops the individual snapshot from being so long it's impossible to understand - if there are changes to tokens - then they should be more obvious now. Before you had to expand the snapshot in github to understand that the token was part of the tokens array. - always write error snapshot, and always write AST/token snapshots - This is just for consistency and to make it easier whilst you're developing. - It will prevent us from accidentally leaving behind error snapshots if they weren't supposed to be there. - added a custom snapshot serializer - Having learned how useful they are with scope manager - I created a new one to improve the look of the snapshots. - Instead of sorting alphabetically, I place `type` at the top, and `range`/`loc` at the end. - Instead of outputting `Object` ahead of every node, instead it outputs the node `type`. - I adjusted the output `range`/`loc` so they take up fewer lines and are more compact. I prefixed the snapshot names with numbers just so we can control the sorting of them. No other reason.

armano2 · 2022-05-06T04:16:50Z

packages/types/tools/copy-ast-spec.ts

+// the promisify util will eat the stderr logs
+async function execAsync(
+  command: string,
+  args: ReadonlyArray<string>,
+  options: childProcess.SpawnOptions,
+): Promise<void> {
+  return new Promise((resolve, reject) => {
+    const child = childProcess.spawn(command, args, {
+      ...options,
+      stdio: 'inherit',
+    });
+
+    child.on('error', e => reject(e));
+    child.on('exit', () => resolve());
+    child.on('close', () => resolve());
+  });
+}


@bradzacher this seem to not be working on windows machines

Error: spawn yarn ENOENT at Process.ChildProcess._handle.onexit (node:internal/child_process:283:19) at onErrorNT (node:internal/child_process:478:16) at processTicksAndRejections (node:internal/process/task_queues:83:21) { errno: -4058, code: 'ENOENT', syscall: 'spawn yarn', path: 'yarn', spawnargs: [ 'build' ] }

and rolling this code back to const execAsync = promisify(childProcess.exec); seem to be fixing it, not sure what is actually causing this issue

If you can figure out how to make it work on Windows and display the error logs - happy to accept a PR!

I'd assume the path isn't properly set so the binary can't be found? That's generally what ENONET means I think.

from my understunding of the issue, its related to / and \ beeing incorrectly used, i did my tests on 2 env win 11 (stable) and win 11 eac (preview), with both node 16 and 18, i was able to reproduce this in all those cases
as for linux subsystem (ubuntu) (linux "vm" in windows) i was unable to reproduce this

edit:
shell, spawn option is required to correctly execute yarn instance (yarn.bat) on windows machines
see: https://stackoverflow.com/questions/37459717/error-spawn-enoent-on-windows

yarn isn't recognized in the console as a program but as a command.

bradzacher added DO NOT MERGE PRs which should not be merged yet tests anything to do with testing labels Apr 1, 2021

bradzacher requested a review from armano2 April 1, 2021 22:28

bradzacher force-pushed the separate-spec-fixtures branch 3 times, most recently from 0a510a2 to 52fb374 Compare April 2, 2021 18:03

bradzacher force-pushed the separate-spec branch 2 times, most recently from 3f04cd5 to a5f8933 Compare May 4, 2021 22:18

Base automatically changed from separate-spec to master May 4, 2021 23:15

bradzacher force-pushed the separate-spec-fixtures branch 6 times, most recently from 63f5b1a to 9edbb00 Compare May 30, 2021 22:42

bradzacher commented May 30, 2021

View reviewed changes

bradzacher force-pushed the separate-spec-fixtures branch from 9edbb00 to e0572c1 Compare May 30, 2021 22:59

bradzacher force-pushed the separate-spec-fixtures branch from 0b491e8 to 1e4b373 Compare September 5, 2021 20:27

bradzacher changed the title ~~feat(ast-spec): add fixtures~~ feat(ast-spec): add fixture test framework and some initial fixtures Sep 10, 2021

bradzacher commented Sep 10, 2021

View reviewed changes

bradzacher force-pushed the separate-spec-fixtures branch 4 times, most recently from 274305a to 7d7e545 Compare October 28, 2021 06:37

bradzacher removed the DO NOT MERGE PRs which should not be merged yet label Oct 28, 2021

bradzacher force-pushed the separate-spec-fixtures branch from 7d7e545 to cab0fd7 Compare October 29, 2021 03:22

bradzacher force-pushed the separate-spec-fixtures branch from 31f3341 to 376997f Compare April 22, 2022 01:59

bradzacher marked this pull request as ready for review April 22, 2022 02:04

JoshuaKGoldberg reviewed Apr 22, 2022

View reviewed changes

packages/ast-spec/tests/fixtures.test.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg previously approved these changes Apr 22, 2022

View reviewed changes

JamesHenry reviewed Apr 24, 2022

View reviewed changes

packages/ast-spec/src/declaration/ClassDeclaration/fixtures/extends-literal/README.md Show resolved Hide resolved

JamesHenry reviewed Apr 24, 2022

View reviewed changes

packages/ast-spec/tests/util/serializers/Node.ts Show resolved Hide resolved

JamesHenry reviewed Apr 24, 2022

View reviewed changes

packages/ast-spec/tests/util/snapshot-diff.ts Show resolved Hide resolved

JamesHenry reviewed Apr 24, 2022

View reviewed changes

packages/ast-spec/tests/util/snapshot-diff.ts Show resolved Hide resolved

bradzacher dismissed JoshuaKGoldberg’s stale review via 7d44794 April 24, 2022 18:04

bradzacher force-pushed the separate-spec-fixtures branch from 376997f to 7d44794 Compare April 24, 2022 18:04

bradzacher force-pushed the separate-spec-fixtures branch from 7d44794 to d699cbf Compare April 24, 2022 18:14

bradzacher requested review from JamesHenry and JoshuaKGoldberg April 25, 2022 05:53

bradzacher force-pushed the separate-spec-fixtures branch from d699cbf to fb13fb5 Compare April 25, 2022 05:56

bradzacher enabled auto-merge (squash) April 25, 2022 06:11

JamesHenry approved these changes Apr 25, 2022

View reviewed changes

bradzacher disabled auto-merge April 25, 2022 19:59

bradzacher changed the title ~~feat(ast-spec): add fixture test framework and some initial fixtures~~ feat(ast-spec): add fixtures Apr 25, 2022

bradzacher changed the title ~~feat(ast-spec): add fixtures~~ feat(ast-spec): add fixture test framework and some initial fixtures Apr 25, 2022

bradzacher merged commit f3cf87b into main Apr 25, 2022

bradzacher deleted the separate-spec-fixtures branch April 25, 2022 20:00

armano2 reviewed May 6, 2022

View reviewed changes

armano2 mentioned this pull request May 9, 2022

test: correct execution of test and dev scripts on Windows machines #4926

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ast-spec): add fixture test framework and some initial fixtures #3258

feat(ast-spec): add fixture test framework and some initial fixtures #3258

bradzacher commented Apr 1, 2021 •

edited

typescript-eslint bot commented Apr 1, 2021

bradzacher May 30, 2021

bradzacher May 30, 2021

bradzacher May 30, 2021

bradzacher May 30, 2021

nx-cloud bot commented Sep 5, 2021 •

edited

bradzacher Sep 10, 2021

bradzacher commented Oct 29, 2021

JoshuaKGoldberg commented Oct 30, 2021

bradzacher commented Apr 22, 2022

JoshuaKGoldberg left a comment

JoshuaKGoldberg Apr 22, 2022

bradzacher Apr 24, 2022

JoshuaKGoldberg Apr 22, 2022

JamesHenry Apr 24, 2022 •

edited

JamesHenry Apr 24, 2022

bradzacher Apr 24, 2022

JamesHenry Apr 25, 2022

JamesHenry commented Apr 23, 2022

bradzacher commented Apr 24, 2022

JamesHenry commented Apr 24, 2022

codecov bot commented Apr 24, 2022 •

edited

armano2 May 6, 2022

bradzacher May 6, 2022 •

edited

armano2 May 9, 2022 •

edited

feat(ast-spec): add fixture test framework and some initial fixtures #3258

feat(ast-spec): add fixture test framework and some initial fixtures #3258

Conversation

bradzacher commented Apr 1, 2021 • edited

Work In Progress

typescript-eslint bot commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nx-cloud bot commented Sep 5, 2021 • edited

☁️ Nx Cloud Report

Choose a reason for hiding this comment

bradzacher commented Oct 29, 2021

JoshuaKGoldberg commented Oct 30, 2021

bradzacher commented Apr 22, 2022

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesHenry Apr 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesHenry commented Apr 23, 2022

bradzacher commented Apr 24, 2022

JamesHenry commented Apr 24, 2022

codecov bot commented Apr 24, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

bradzacher May 6, 2022 • edited

Choose a reason for hiding this comment

armano2 May 9, 2022 • edited

Choose a reason for hiding this comment

bradzacher commented Apr 1, 2021 •

edited

nx-cloud bot commented Sep 5, 2021 •

edited

JamesHenry Apr 24, 2022 •

edited

codecov bot commented Apr 24, 2022 •

edited

bradzacher May 6, 2022 •

edited

armano2 May 9, 2022 •

edited