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

Doesn't support it.each tests #2

Closed
haydenmeade opened this issue Jun 20, 2022 · 13 comments · Fixed by #51
Closed

Doesn't support it.each tests #2

haydenmeade opened this issue Jun 20, 2022 · 13 comments · Fixed by #51

Comments

@haydenmeade
Copy link
Collaborator

it.each([...])(...)

@adrigzr
Copy link
Collaborator

adrigzr commented Jun 28, 2022

I've already investigated this. We need first neotest to add some kind of helper to catch params passed to each(params) in order for us to build the Tree.id with it. In any case, it seems like hard work to parse and build the id by hand replicating Jest.

@haydenmeade
Copy link
Collaborator Author

Wondering if we can build string formatted test names in adapter.discover_positions(path).

@adrigzr
Copy link
Collaborator

adrigzr commented Jul 11, 2022

Could you provide an example of string formatted test names?

@haydenmeade
Copy link
Collaborator Author

Not at pc right now, but https://jestjs.io/docs/api#testeachtablename-fn-timeout

name: String the title of the test block.

Generate unique test titles by positionally injecting parameters with printf formatting:

%p - pretty-format.

%s- String.

%d- Number.

%i - Integer.

%f - Floating point value.

%j - JSON.

%o - Object.

%# - Index of the test case.

%% - single percent sign ('%'). This does not consume an argument.


@haydenmeade
Copy link
Collaborator Author

See #21 for unit test.
I think we could either:

  • Format test name in treesitter parse
  • Match to correct test when parsing results

I prefer the 1st option since you would see the correct names in the test sidebar.

@adrigzr
Copy link
Collaborator

adrigzr commented Jul 12, 2022

Yep. This implies implementing the Jest interpolation logic by ourselves in the parsing method, but I don't see any other valid way...

@adrigzr
Copy link
Collaborator

adrigzr commented Jul 12, 2022

I'm taking a look at the code and we need to change the treesitter lib of neotest. Something like this:

diff --git a/lua/neotest/lib/treesitter/init.lua b/lua/neotest/lib/treesitter/init.lua
index 26b7e8c..8b44f6a 100644
--- a/lua/neotest/lib/treesitter/init.lua
+++ b/lua/neotest/lib/treesitter/init.lua
@@ -40,11 +40,13 @@ local function collect(file_path, query, source, root)
       ---@type string
       local name = vim.treesitter.get_node_text(match[query.captures[type .. ".name"]], source)
       local definition = match[query.captures[type .. ".definition"]]
+      local args = match[query.captures[type .. ".args"]]
 
       nodes:push({
         type = type,
         path = file_path,
         name = name,
+        args = args and vim.treesitter.get_node_text(args, source) or nil,
         range = { definition:range() },
       })
     end

Then we can use it with this query

    ; Matches: `test.each(['data'])('test') / it.each(['data'])('test')`
    ((call_expression
      function: (call_expression
        function: (member_expression
          object: (identifier) @func_name (#any-of? @func_name "it" "test")
        )
        arguments: (arguments (_) @test.args)
      )
      arguments: (arguments (string (string_fragment) @test.name) (arrow_function))
    )) @test.definition

Then, we can override the position_id option to parse the arguments as JSON and interpolate test names.

We also need to implement something in neotest to create multiple test node using one single node...

@haydenmeade
Copy link
Collaborator Author

Sounds like a reasonable approach to me

@prudnikovsv
Copy link

Is there any news about this issue? Its one of the featured that is used over the place, and not to be able to run this kind a tests is a big con.

@haydenmeade
Copy link
Collaborator Author

There were a few solutions proposed in nvim-neotest/neotest#68. I haven't had time to look into it, but it would be great if someone could :)

@KostkaBrukowa
Copy link
Contributor

Hey!
I thought that I could tackle this and I want to present you my findings.
So basically there are two approaches to solving this issue:

  1. Go with the treesitter and try to "guess" the parameters of the test and connect it with the jest test results
  2. Actually run the jest command and get the test names from jest output

At first I've tried to go with the first approach, because treesitter compared with jest is pretty much instant but I've stumbled upon the problem that was described in detail in nvim-neotest/neotest-python#35. Long story short: treesitter cannot really "guess" test names of tests that contain dynamic (evaluated at runtime) values. e.g.

    it.each([SomeEnum.Value])('should allow number values %s', (param) => {
      expect(number).toBe(1);
    });

SomeEnum.Value could be anything there (number or a string) and we would require run this javascript file to find out the value. Moreover in this case:

    it.each(someImportedData)('should allow number values %s', (param) => {
      expect(number).toBe(1);
    });

we could't even guess number of tests - this prevents us from connecting the tests by their order in tests results.

Therefore I went with second approach and I would like to know your opinion on this. This is the algorithm that I went for in my PR (#51):

  1. Run the treesitter queries on the test file.
  2. If the file do not have any parameterised tests (it.each, test.each etc.) do not do anything special just return normal test positions
  3. If file contains parameterised tests then:
  • run jest to discover every test in the file (ad.1 below on how I did it)
  • find all of the test results from jest that are on the same positions as tests returned by treesitter
  • add new positions to the position returned by a treesitter

With this approach we are certain to have correct every test name in the summary window.
Unfortunately this approach is slower that "guessing" solution, but it gives us certainty that we have all the tests with correct names.

I've linked a WIP PR to see what do you think about this approach.

Ad. 1
Unfortunately jest doesn't have the flag to only get test names from the test file so I came up with the idea to add extra parameter to the command that should never exist in the test file (https://github.com/haydenmeade/neotest-jest/pull/51/files#diff-b06c5b2f8aca9ebc9cb4593b810b58661b424e3b97a8fcf141061761d191b3d4R46). If you add this parameter jest just basically skips all of the tests in the file, but still has the test names in the result.

@dsuttonpreece
Copy link

@KostkaBrukowa very interested in where you're up to with this. Would be happy to help if you don't have time to address the bugs mentioned in here #51

@KostkaBrukowa
Copy link
Contributor

Sorry for late replies. I've replied in the PR, but unfortunately I'm no longer working with jest (globally switched to vitest) anymore so I don't have the motivation and cannot really test my solution in real life job. Therefore if you or anyone is interested in completing the PR feel free to do that

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

Successfully merging a pull request may close this issue.

5 participants