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

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

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

V13Axel
Copy link
Contributor

@V13Axel V13Axel commented Feb 7, 2024

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. 😅

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
Copy link
Owner

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

@V13Axel
Copy link
Contributor Author

V13Axel commented Feb 16, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

V13Axel commented Feb 16, 2024

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
Copy link
Contributor Author

V13Axel commented Feb 16, 2024

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 olimorris merged commit 0eab70d into olimorris:main Feb 20, 2024
4 checks passed
@olimorris
Copy link
Owner

Great. Thanks so much for this!

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 this pull request may close these issues.

2 participants