thenbe / neotest-playwright

A playwright adapter for neotest.
MIT License
37 stars 5 forks source link

Discussion: Automatically filter out directories based on playwright config #24

Open AlexandrosAlexiou opened 6 months ago

AlexandrosAlexiou commented 6 months ago

Would be great if we could filter out directories based on the testDir directive in the playwright configuration.(docs)

thenbe commented 6 months ago

While that is something we can do, I'm not sure I can see the case for actually doing so (given the overhead of running the playwright cli). Do you currently feel the need to always define filter_dir? If so, why? What happens if you leave it as default?

I ask because filter_dir is generally not something that users are expected to customize a whole lot and I'd like to make sure this is not a side effect of a different issue.

Also, what does your neotest-playwright config generally look like?

AlexandrosAlexiou commented 6 months ago

Hello, first of all thanks for the insightful questions.

In my workflow I use neotest-vitest and neotest-playwright. My playwright config is more or less like this and I specify my testDir.

The issue is that the vitest plugin discovers test using this function, and it will mess it up and think that playwright tests are vite tests and vice versa, because test files between these two libraries might have same file extensions.

So telling these plugins which folders to use (using filter_dir) seems like a solution.

Any thoughts on that?

thenbe commented 6 months ago

I haven't used other neotest adapters that much so I had not previously thought about how neotest would behave in that scenario. But I can see how that would be an issue.

After #26 is merged, one way to address it would be to use a different extension for each. For instance, you could use foo.test.ts for playwright tests and foo.spec.ts for vitest. Then you'd customize neotest-playwright's is_test_file to reject the *.spec.ts pattern, leaving it for neotest-vitest to handle.

Additionally, there's always the option of using a different directory (say a test directory for vitest and a test-e2e for playwright). In that case, you can just customize filter_dir instead.

AlexandrosAlexiou commented 6 months ago

I haven't used other neotest adapters that much so I had not previously thought about how neotest would behave in that scenario. But I can see how that would be an issue.

After #26 is merged, one way to address it would be to use a different extension for each. For instance, you could use foo.test.ts for playwright tests and foo.spec.ts for vitest. Then you'd customize neotest-playwright's is_test_file to reject the *.spec.ts pattern, leaving it for neotest-vitest to handle.

Additionally, there's always the option of using a different directory (say a test directory for vitest and a test-e2e for playwright). In that case, you can just customize filter_dir instead.

Sure, If the project is a personal one I can follow these practices with file extension naming conventions, but unfortunately not everyone follows conventions and just name all tests (unit, e2e) ending with tests.ts which messes up with the adapters.

sand4rt commented 6 months ago

but unfortunately not everyone follows conventions and just name all tests (unit, e2e) ending with tests.ts

I think that depends on the type of project. When you have Playwright component testing and Vitest units in the same folder you must differentiate the Vitest and Playwright tests with by file extension otherwise Vitest and Playwright will collide:

/MyProject
/MyProject/package.json

/MyProject/vitest.config.ts <-- defineConfig({ includes: ['src/**/*.{test,spec}.?(c|m)[jt]s?(x)'] })
/MyProject/playwright.config.ts <-- defineConfig({ testDir: 'e2e' })
/MyProject/playwright-component-testing.config.ts <-- defineConfig({ testDir: 'src' })

/MyProject/src/login.component.test.ts
/MyProject/src/login.unit.test.ts
/MyProject/e2e/login.test.ts

It does not make much sense to add the file extension in this scenario tho, where the e2e tests are in a separate folder:

/MyProject
/MyProject/package.json

/MyProject/vitest.config.ts
/MyProject/playwright.config.ts

/MyProject/src/login.test.ts
/MyProject/e2e/login.test.ts
thenbe commented 6 months ago

@sand4rt Replying to your comment here as I'm afraid Github will bury that thread after that PR is merged.

Thanks for the explanations! At least it's only needed in certain situations like resolving adapter collisions, that makes it less laborious than i thought. I still wonder if it isn't better to automatically discover the tests somehow?

Maybe by parsing the playwright.config.ts file associated with the registered test adapter/project and read properties like testMatch or testDir? I believe the vscode extension accomplishes something similar.

When I'm working on projects with both Vitest unit tests and Playwright component tests, I usually have to specify defineConfig({ testMatch: '**/?(*.pw.)+(spec|test).ts }) for Playwright, and/or defineConfig({ test: { include: ['**/*.unit.{test,spec}.?(c|m)[jt]s?(x)'] } }) for Vitest _(AKA what is proposed to use in is_test_file)_. Can't this be reuse this somehow?

I haven't tried it but I it should be possible to re-use that info. When we call the playwright cli, it returns this json object which contains the entire resolved config, including testMatch and testDir for all projects.

I suppose we could then apply those values to our adapter. I'm not sure if the glob from testMatch needs to be translated to something nvim understands before being used in is_test_file. If so, maybe vim.glob can help.

Aside from that, there a couple of things that are still not clear to me:

zettadam commented 6 months ago

Here is my setup. I have neotest-vitest & neotest-playwright plugins installed and "configured" (LazyVim).

My Vitest tests are in './src' directory (with *.spec.tsx extension) and my Playwright tests are in ./tests directory (with *.test.ts extension).

At the moment I don't see a way to run Playwright tests separately using neotest. When I press <leader>tT (run all tests) key combo, neotest-vitest runs all tests and consequently all Playwright tests fail.

Screenshot 2024-02-25 at 8 31 51 PM Screenshot 2024-02-25 at 8 32 23 PM

I wish there was a simple way to configure both to run separately, perhaps even using different key bindings, since to my mind this is a very common scenario in a non-hobby environment.

thenbe commented 6 months ago

At the moment I don't see a way to run Playwright tests separately using neotest.

@AlexandrosAlexiou has recently added PR's to both neotest-playwright and neotest-vitest that expose a new setting is_test_file to the user config. This should give you the extra control you need to finely tune the adapters' behaviors as to which files each should be responsible for.

Can you confirm that this allows you to run both tools simultaneously without having them trip over each other?

AlexandrosAlexiou commented 6 months ago

At the moment I don't see a way to run Playwright tests separately using neotest.

@AlexandrosAlexiou has recently added PR's to both neotest-playwright and neotest-vitest that expose a new setting is_test_file to the user config. This should give you the extra control you need to finely tune the adapters' behaviors as to which files each should be responsible for.

Can you confirm that this allows you to run both tools simultaneously without having them trip over each other?

Yeap that's right, I had the same issue with adapters overlapping. I have configured it using the is_test_file and now it works fine. Please let us know if something is not working for you.

The only issue that I have now is with neotest's test summary. Basically only files within the first registered adapter will trigger the test summary unfold when opened in a buffer.

zettadam commented 6 months ago

I modified my earlier configuration for neotest-playwright to look like this:

return {
  "nvim-neotest/neotest",
  dependencies = {
    -- "nvim-lua/plenary.nvim",
    -- "nvim-treesitter/nvim-treesitter",
    "marilari88/neotest-vitest",
    "thenbe/neotest-playwright",
  },
  opts = function(_, opts)
    table.insert(
      opts.adapters,
      require("neotest-vitest")({
        filter_dir = function(name)
          return name ~= "node_modules" or name ~= "tests"
        end,
      })
    )
    table.insert(
      opts.adapters,
      require("neotest-playwright").adapter({
        options = {
          persist_project_selection = true,
          enable_dynamic_test_discovery = false,
          is_test_file = function(file_path)
            return string.match(file_path, "tests")
          end,
        },
      })
    )
  end,
}

My playwright tests are in tests directory, but neotest-vitest continues to run them and they fail.

Please document it with a realistic looking example.

thenbe commented 6 months ago

My playwright tests are in tests directory, but neotest-vitest continues to run them and they fail.

A neotest adapter can only control what it runs. A neotest adapter can not control which tests other adapters decide to run.

If neotest-playwright is running tests where it should not, I'll try to help. But if neotest-vitest is the one running tests where it should not, you should open an issue in the neotest-vitest repo.

As far as neotest-playwright is concerned you should still amend is_test_file to a function that returns true if and only if file_path ends with *.test.ts (that's the extension you use for playwright tests according to your earlier posts). Doing that will force neotest-playwright to ignore other file patterns it would have otherwise slurped up.

thenbe commented 2 months ago

More strict examples for is_test_file have been added in 985ec2649831.