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

feat: add support for Pest in vapor test #233

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

owenvoke
Copy link
Contributor

@owenvoke owenvoke commented Sep 13, 2023

This adds support for Pest when using the vapor test command. It does this based on the existance of the vendor/bin/pest binary, however, I'm not sure whether we want to use a --pest flag (which was my initial attempt, but it seemed awkward with the array_splice(), and I didn't want to make too many changes).

@taylorotwell taylorotwell marked this pull request as draft September 14, 2023 13:05
@joedixon
Copy link
Contributor

Thanks for sending this in @owenvoke, I think it makes a lot of sense to support Pest here.

I'm in the process of jumping on the PR as I think I'd prefer to try and make the --pest flag work, but I can't actually get my tests to run using it.

Did you manage to successfully test the PR locally?

@owenvoke
Copy link
Contributor Author

owenvoke commented Sep 14, 2023

Hi @joedixon 👋🏻 I managed to test this successfully locally, I basically just went to another project using Pest and ran vendor/bin/vapor test 🤷🏻

What error are you getting?

Actually, it's not working, because it's running the vendor/bin/pest (and vendor/bin/phpunit) command in /var/www/html rather than /app 🤔 I think this is because php:*-fpm-alpine mounts /var/www/html as the working directory, whereas the local command maps to /app. But not sure if that's just my issue.

@joedixon
Copy link
Contributor

This is the same issue I'm seeing. I can fix this by adding -w /app to the docker-compose command, but just checking to see if that breaks anything else.

@joedixon
Copy link
Contributor

@owenvoke just pushed some updates to support the --pest option. Would you mind giving it a test?

Summary of changes:

  • Set working directory to /app - I can't see this being an issue as the only commands to use this are test which needs to run from that directory and local for running commands which again need to run from the project root
  • Remove everything but the command and the executable from ARGV server variable
  • Use array_search to deteremine the presence of php option in the local command

The last point is not bullet proof as the arguments have to be in the correct order (e.g. vapor local --php=8.2 "ls -la"), but I believe this works in the same way it did prior to this update.

@owenvoke
Copy link
Contributor Author

owenvoke commented Sep 15, 2023

I've tested this change locally, and it seems to work great. 🙌🏻 My only thought is the same as your last point, I'm not a huge fan about requiring specific orders. But not sure of a good solution to this. 🤷🏻

As far as I can tell, this will work in the same way as before. 👍🏻

@joedixon joedixon marked this pull request as ready for review September 15, 2023 12:45
@taylorotwell taylorotwell merged commit 7eff841 into laravel:master Sep 15, 2023
19 checks passed
@owenvoke owenvoke deleted the feature/pest branch September 15, 2023 22:19
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.

3 participants