Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parse does not find name for test.each block #29

Closed
firsttris opened this issue Jan 21, 2020 · 12 comments
Closed

parse does not find name for test.each block #29

firsttris opened this issue Jan 21, 2020 · 12 comments

Comments

@firsttris
Copy link
Contributor

firsttris commented Jan 21, 2020

      test.each([
        [1, 1, 2],
        [1, 2, 3],
        [2, 1, 3]
      ])('wtf', (a, b, expected) => {
        expect(a + b).toBe(expected);
      });

https://jestjs.io/docs/en/api#testeachtablename-fn-timeout
(note: its not possible to put a test.each inside a "it" or a "test" )

parser outputs a warning if he detects such a block.

  console.warn node_modules/jest-editor-support/build/parsers/typescript_parser.js:96
    **NamedBlock but no name found for it tsNode= NodeObject** {
      pos: 442,
      end: 526,
      flags: 0,
      modifierFlagsCache: 0,
      transformFlags: 0,
      parent: undefined,
      kind: 195,
      expression: NodeObject {
        pos: 442,
        end: 459,
        flags: 0,
        modifierFlagsCache: 0,
        transformFlags: 0,
        parent: undefined,
        kind: 193,
        expression: IdentifierObject {
          pos: 442,
          end: 454,
          flags: 0,
          modifierFlagsCache: 0,
          transformFlags: 0,
          parent: undefined,
          kind: 75,
          escapedText: 'test'
        },
        questionDotToken: undefined,
        name: IdentifierObject {
          pos: 455,
          end: 459,
          flags: 0,
          modifierFlagsCache: 0,
          transformFlags: 0,
          parent: undefined,
          kind: 75,
          escapedText: 'each'
        }
      },
      questionDotToken: undefined,
      arguments: [
        NodeObject {
          pos: 460,
          end: 525,
          flags: 0,
          modifierFlagsCache: 0,
          transformFlags: 0,
          parent: undefined,
          kind: 191,
          multiLine: true,
          elements: [Array]
        },
        pos: 460,
        end: 525
      ]
    }
@firsttris
Copy link
Contributor Author

the parsed object for this block:

{
                                        "type": "it",
                                        "file": "filename...",
                                        "start": {
                                             "column": 7,
                                             "line": 22
                                        },
                                        "end": {
                                             "column": 37,
                                             "line": 26
                                        }
                                   },
                                   {
                                        "type": "expect",
                                        "file": "filename...",
                                        "start": {
                                             "column": 9,
                                             "line": 27
                                        },
                                        "end": {
                                             "column": 37,
                                             "line": 27
                                        },
                                        "children": [
                                             {
                                                  "type": "expect",
                                                  "file": "filename...",
                                                  "start": {
                                                       "column": 9,
                                                       "line": 27
                                                  },
                                                  "end": {
                                                       "column": 37,
                                                       "line": 27
                                                  }
                                             }
                                        ]
                                   }

@seanpoulter
Copy link
Member

Looks like you've done a great job identifying the root cause of the problem. How would you feel about trying to update the parser @firsttris?

@firsttris
Copy link
Contributor Author

hey @seanpoulter
thx for your reply.

i will take a look at extending the parser. this should be something im capable of

just a question about this comment:
https://github.com/jest-community/jest-editor-support/blob/master/src/parsers/typescript_parser.js#L9
Does this mean there is another (probably newer?) jest parser in the jest repository?
or
Is the parser of jest-editor-support the best choice for a jest parser in a vscode addon?

@seanpoulter
Copy link
Member

📣 Quick question for the maintainers/contributors: Are we good with a major/breaking change to consolidate the test parsers into only one using Babel 7? That'd drop the TypeScript parser. Thanks! 👍

--

Those are great questions @firsttris! The comment in typescript_parser.js is from @connectdotz's epic PR #9 🎉. I'll try to tell the story again in my own words for context:

  • This package used to be in the facebook/jest repo.
  • The TypeScript test parser was also a separate package in the facebook/jest repo.
  • This repo was moved from facebook/jest packages/jest-editor-support to the jest-community/jest-editor-support, and we also needed a new home for the TypeScript parser.
  • If you have a look at PR refactoring prepare for vscode-jest integration #9 you'll get a sense there was a lot going on. @connectdotz kept the JavaScript (Babel) and TypeScript parsers as is but pointed out there's an option to improve this:

    babel-7 has a typescript plugin that could potentially replace this once we get around to test it.


Does this mean there is another (probably newer?) jest parser in the jest repository?

The "new" parser is actually the ability for Babel to parse TypeScript. That means we can remove the TypeScript parser and handle both types of files with Babel. It was one of the big changes that was introduced with the release of Babel 7.

I don't think there's a parser in the Jest repository any more. I had a quick look and the only hints I could find were to parse the snapshots.


Is the parser of jest-editor-support the best choice for a jest parser in a vscode addon?

Is it the best choice of parser? You may not like my answer: "it depends". We've been maintaining the parser for the few use cases that we can think of, mostly for vscode-jest. As you've discovered, we haven't been doing the best job keeping up with changes or building the team of folks to help maintain it. Despite that, the parser has been stable for quite a while. I can say that when I was able to use Jest and the VS Code Jest extension, I was really happy with it.

If you're looking around at other test parsers, @Raathigesh has his own parser for majestic: https://github.com/Raathigesh/majestic/blob/master/server/services/ast/parser.ts

--

Have you worked with the abstract syntax tree (AST) or a parser before? I've found http://astexplorer.net really helpful to experiment before I had Babel working from the CLI.

@firsttris
Copy link
Contributor Author

firsttris commented Jan 24, 2020

@seanpoulter now i have a much better understanding.

My understanding is that majestic is also using @babel/parser which is the parser of Babel 7.x
https://babeljs.io/docs/en/babel-parser

I was just thinking if i could do the parsing the same way that Jest was doing it, it would probably be the best option. Is Jest also using the babel/parser?

thx for your help

@connectdotz
Copy link
Collaborator

@seanpoulter did a pretty good explanation. Yes, we do want to replace the custom typescript parser with a more maintained one, like babel-parser, provided:

  1. it can continue to support the current clients' need, such as vscode-jest, i.e. integration testing from vscode-jest side is needed before adopting the new parser.

  2. it adds value. As @seanpoulter pointed out, it is stable and does most what we need. Well, except the template literal and dynamic strings. Before we jump into the work to replace it, I think we should do a bit more research in this area to make sure the new parser can indeed resolve these issues...

    This is a static parser, unlike jest, it doesn't run the code. This means it is hard for the parser to resolve anything that requires the run-time context, such as template-literal strings. Do you know if babel-parser can resolve these kinds of dynamic strings during traversing?

@firsttris
Copy link
Contributor Author

Just wanted to give a little background why i even extended your parser:

I'm the author of vscode-jest-runner.
If you search for jest in marketplace my extension is second one. (after vscode-jest)
I have 41k installs which is still 6 times less then vscode-jest.
vscode-jest-runner is about running or debugging one test. (e.g. to track down bugs)
While vscode-jest is about running all tests in your current file in a watch flow. (correct me if im wrong)
(I do not see any competition since its a different workflow.)

At some point with my extension i wanted to be able to contruct a full unqiue test path.
(To not run duplicate tests)

something like:

jest -t "describeName childDescribeName testName"

I started searching for a Jest parser. After some time i found jest-editor-support.

Some while ago a user created a bug that "test.each" is not working.
firsttris/vscode-jest-runner#55
After some research i found that its not implemented in your parser.
Means vscode-jest will most probably have the same issue.

Since @seanpoulter was so kind and explained me everything.
I thought why not help out?

Extending two parser (babylon and typescript) with test.each seemed not the best option?
Also @seanpoulter pointed me in the direction to unify them with the babel/parser.

Did not run into issues with "template literal and dynamic strings" with my extension yet.
But i found possible evidence in the parser of majestic
https://github.com/Raathigesh/majestic/blob/master/server/services/ast/inspector.ts#L109

Probably a test could verify this?

thx for your help
Tristan

@firsttris
Copy link
Contributor Author

I saw there is already a test file for template-literals in fixtures/template-literal.example.
That probably means your issue is only runtime specific.

Small runtime template-literal Test:

const myRunTimeValueString = `eat this ${process.env.USER} babel parser!`;
console.log(myRunTimeValueString);

process.env.USER was "firsttris" during runtime

i let babel/parser parse this

result:
https://gist.github.com/firsttris/17116b9dd2d2037805fda8fd68163b6b

it was not able to resolve my username.

does this answer your question about template-literal strings with runtime context?

@connectdotz
Copy link
Collaborator

@firsttris thanks for the context. Apology if I came across as unappreciative. Indeed the unselfish act of making open-source repo better, even during our weekends 😏, is what made the community so great! 😄

Anyway, come back to the typescript parser issue.

I was just thinking if i could do the parsing the same way that Jest was doing it, it would probably be the best option. Is Jest also using the babel/parser?

Jest doesn't use this package directly. It was able to resolve all the dynamic names etc because it is actually running the tests, not parsing the code.

it was not able to resolve my username. does this answer your question about template-literal strings with runtime context?

Yes, it does. And I also looked at the majestic's inspector code, it didn't seem to resolve the variable either.

Did not run into issues with "template literal and dynamic strings" with my extension yet.

We do and I think you will too, eventually. Even jest.each supports that.

Anyway, I am pretty convinced now that the dynamic strings will not be resolved in a static parser. I shall drop that as a parser enhancement wish-list...

As I mentioned in the comment, it is always our desire to move to babel's, actually, we almost did that when this package is still part of jest repo... Change in this package might not be hard but I know the compatibility testing and potential interruption of the user experience might be significant (will add more about this in your PR), therefore we didn't pull the trigger then...

But now I saw PR #32 for typescript conversion, another item in my personal wish list, I started to think, if there is no strong objection (@stephtr @seanpoulter), maybe this is a good time to consider a new major release? If we have to do a lot of integration tests for each change, we might as well do them at the same time...?

@botsman
Copy link

botsman commented Apr 15, 2020

Hey guys!

I am using a VS Code extension which depends on this project.

Could you tell me, what is the status of this issue and related PR?
It would be really awesome to have it merged, since it solves my problem with the test.each block (parametrized tests).

Thank you!

@seanpoulter
Copy link
Member

Hey @pbotsman. Well, it looks like that PR has been stalled for over a month. If you've got the interest and capacity, I'm sure we'd all appreciate your help having a look with fresh eyes and "managing" it to get it back on track. What's left to do? I can't assign you as a reviewer (yet). 😉

@firsttris
Copy link
Contributor Author

i think this isuse is resolved now, please re-open if im wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants