Update .env.ci and .env.testing APP_ENV#729
Conversation
… allow overwriting to local
Bumps [symfony/html-sanitizer](https://github.com/symfony/html-sanitizer) from 7.4.7 to 7.4.13. - [Release notes](https://github.com/symfony/html-sanitizer/releases) - [Changelog](https://github.com/symfony/html-sanitizer/blob/8.1/CHANGELOG.md) - [Commits](symfony/html-sanitizer@v7.4.7...v7.4.13) --- updated-dependencies: - dependency-name: symfony/html-sanitizer dependency-version: 7.4.13 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
@bogdankharchenko here's the original concern. I've copied the comment below since the link jumping seems to be doing goofy things.
Our .env.ci does declare the 'local' environment and the workflow file is running
If I recall, the test_robots_txt_disallows_all_in_non_production_test was failing on Github saying that 'local' didn't match 'testing'. Before we do anything else, it's probably worth verifying if that's the case by emitting the environment variable as part of the testing workflow to find out if Github is running tests in 'local' (via to the .env.ci copying) or 'testing' (via phpunit.xml).
Note that I changed test_robots_txt_disallows_all_in_non_production_test in #726 so it checks for 'not production' instead of specifically for 'testing'. That change allowed for running Here's test_robots_txt_disallows_all_in_non_production_test as it is now. This test would pass regardless of if Github is running in 'testing' or 'local', but that's by design as we expect to see Disallow in any env that's not production.
|
|
I'm not sure about this change, to me needing to add something that says |
|
@oliviasculley @bogdankharchenko the main question is if we are intentionally importing .env.ci into the workflow. It declares local, which seems odd to me. I would have assumed 'testing' If that's the fix, then I agree that we don't need this PR, but rather a PR to change .env.ci to testing. |
|
I just read a little more and I think we should definitely be modifying the |
|
I'm pulling @irby back in as the .env.ci was created by Matt with 'local' in the original file. I feel like he thought the phpunit.xml change was okay, but it could have been that I should have suggested a better solution, like could we "fix" the .env.ci, or should we change the Github workflows to use the .env.testing. The .env.ci and .env.testing both use I'm not a testing guru, so I'm not even sure if/why we need both and if we could simply change .env.ci or .env.testing to If that's the easy fix, then we'd also want to compare .env.testing and .env.ci because the former is probably out of date. Side note, since I looked it up. There is a lot of overriding that can happen (summary below), so I agree the PHPUnit thing feels sort of ick if we can fix it at another level.
|
|
lets update |
|
@bogdankharchenko I'll do that. What about .env.testing? It's also local. Do we want / need both files? |
…la/hackgreenville-com into tweak/force-phpunit-to-testing-env
|
Does anybody know when we're using .env.testing? |
|
|
|
@oliviasculley the confusion I'm having is that we are importing .env.ci in the main workflow. The .env.testing has diverged a bit from .env.ci and it's still unclear if Github is using .env.testing or if that's an artifact. It looks like @zach2825 created .env.testing in Feb 2021 and this file appears to be a Laravel convention. @irby created .env.ci in July 2023, presumably as a Github naming convention.
If both have a purpose, then we could document that in CONTRIBUTING.md, which currently just says Maybe .env.ci for Github and .env.testing for Laravel php artistan test --env=testing runs? It's clear when the .env.ci is being used, but not where in the current Labs flow where .env.testing could be used. |
|
Hey @allella, so there's actually a bug in our current GH Actions setup that's worth understanding before we make any changes. The If we set So we have two options here: Option 1: Run tests against MySQL
Option 2: Drop MySQL from CI entirely
My recommendation is Option 1. We already pay the cost of running MySQL in CI; we should get the benefit of actually testing against it. @bogdankharchenko what are your thoughts? |
|
I'm also in favor of running MySQL in ci, we want to test with MySQL quirks since we're using that in production |
|
I decided to double check this with Opus 4.8 as well
Also I'd like to see all of this documented in the readme/docs somewhere |
|
The .env files and docs have been tweaked based on the above suggestions. Thanks, and please give this PR a fresh review when you have a moment. |
|
Looks great! |
|
Welp, the latest tests from the homepage rework started failing as soon as this was merged. I opened #750 with more details. |




Per conversation in #726 we had support for forcing PHPUnit to use testing.
The Github action that runs our tests is copying .env.ci and that's set to 'local', which appears to preempt the phpunit.xml value.