search

olimorris / neotest-phpunit

🧪 Neotest adapter for PHPUnit
MIT License
29 stars 22 forks source link

Add option to _ignore_ a project root based on file existence #19

Closed V13Axel closed 6 months ago

V13Axel commented 7 months ago

This PR adds a new config option, root_ignore_files, a table of files whose presence mean a project should not be considered a phpunit project. My main goal here is to prevent neotest-phpunit from claiming projects setup for Pest.

You may already be familiar, but just in case: Pest uses PHPUnit under the hood. As such, phpunit.xml exists. However, its tests are not compatible with PHPUnit's - so running phpunit directly in a Pest project will just crash.

Essentially, I've forked neotest-pest, updated it for the latest version of pest, and started using it. It's just that sometimes when I hit my run() key, neotest-phpunit seems to "speak up" first and decides my tests are failing (due to the aforementioned crash). I wind up needing to manually select neotest-pest in the summary window or some other janky workaround. :sweat_smile:

However, since Pest's setup adds a file tests/Pest.php, that's where this new root_ignore_files option comes in. It can be set to root_ignore_files = { "tests/Pest.php" } and neotest-phpunit simply ignores the project.

olimorris commented 7 months ago

Great addition! Can you update the readme to document this too?

V13Axel commented 7 months ago

Sure thing @olimorris ! I'll get that updated shortly. While I'm at it, any opposition to setting a default of tests/Pest.php?

olimorris commented 7 months ago

Sure thing @olimorris ! I'll get that updated shortly. While I'm at it, any opposition to setting a default of tests/Pest.php?

My preference would be to not set any defaults and let users "opt-in" if that makes sense. I'd want to cover the use case of users not reading the README and installing it outright and not getting expected results...then consulting the README.

V13Axel commented 7 months ago

I totally understand the preference, and will ultimately defer to your judgement, but I do want to reiterate just in case it wasn't clear: Vanilla phpunit tests (and thus this adapter) will not work in a project using Pest - So everyone who has both Pest projects and PHPUnit projects (and thus both adapters) that they need to support will need to set the ignore config.

As such, I'd generally think the "expected results" would be for the adapter to avoid projects that it is guaranteed not to support.

If we really want to point those folks to the readme, perhaps pointing them to the appropriate adapter for Pest alongside the config note? Something like:

If there are projects you don't want discovered, you can set root_ignore_files to ignore any matching projects.

For example, if your project uses Pest and the matching neotest adapter, you'll want to set:

require("neotest-phpunit")({
    root_ignore_files = { "tests/Pest.php" }
})

Thoughts?

V13Axel commented 7 months ago

I've gone ahead and added a note to the readme @olimorris, would love to hear any thoughts on the above though - The mutual exclusivity of the adapters was my main driver in submitting this PR, so I think a default is at least worth considering, to save others some time, configuration, and/or confusion. Thanks either way!

olimorris commented 6 months ago

Great. Thanks so much for this!