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

Test wildcard environments config #1465

Merged
merged 3 commits into from
Jun 17, 2024
Merged

Conversation

karkowg
Copy link
Contributor

@karkowg karkowg commented Jun 13, 2024

I might be looking at this totally wrong but I believe ProvisioningPlanTest::test_supervisors_are_added_by_wildcard isn't testing anything related to wildcard environments. It just looks like a duplicate of ProvisioningPlanTest::test_supervisors_are_added.

I came by this because each member of my team is using a different APP_ENV and I was looking for a simple way to declare that in the Horizon config without having to list them all. I thought adding a wildcard environment wasn't possible, but it is, only missing documentation. If my assumptions are correct, here's a fix to that test. I've also sent laravel/docs#9706 for documenting the wildcard usage.

Thanks!

@taylorotwell
Copy link
Member

Can you add an entirely new test that you think is properly setup?

@taylorotwell taylorotwell marked this pull request as draft June 14, 2024 01:38
@karkowg
Copy link
Contributor Author

karkowg commented Jun 14, 2024

Can you add an entirely new test that you think is properly setup?

What would be the benefit of having a new test?

If you diff test_supervisors_are_added and test_supervisors_are_added_by_wildcard, the only difference is the function name.

Don't you think the change is proper in that case? If not, I'm not seeing what that test is doing in its current form.

@driesvints
Copy link
Member

@karkowg could you please just add the new test as Taylor requests? It's better to leave the existing tests alone.

@karkowg
Copy link
Contributor Author

karkowg commented Jun 17, 2024

@driesvints yep, no problem. I was just trying to understand what I would have to do differently if adding a new test.

@karkowg karkowg marked this pull request as ready for review June 17, 2024 13:25
@taylorotwell taylorotwell merged commit a45917a into laravel:5.x Jun 17, 2024
10 checks passed
@karkowg karkowg deleted the wildcard-env-test branch June 17, 2024 15:24
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