-
Notifications
You must be signed in to change notification settings - Fork 502
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
Streamline local test environment setup #916
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
Will still need a note in CONTRIBUTING that this will not work on Windows for now.
As discussed, not merging yet until a shellcheck check has been added in CI to safeguard the scripts.
Future scope: add a Docker config file to get up & running with even less effort (and which would work on Windows too). |
Just noticed, just running the script on my machine created some |
I changed the folder where PIDs are stored and added that location to |
60f07ce
to
f09ca3e
Compare
Just checking - this PR relates to #819 (and closes it ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schlessera I've had a good look at this PR, with the caveat of limited knowledge of bash scripting...
Left a number of questions in-line.
Additionally:
- The scripts re-introduce a problem we previously solved: the system default PHP version being used instead of the Composer PHP version being used. This is a regression, which I'm not happy with.
/scripts/
directory needs to be added to.gitattributes
composer.json
: script descriptions should mention that this doesn't work on Windows, only on *nixCONTRIBUTING
: should mention that this doesn't work on Windows, only on *nix.CONTRIBUTING
: "The environment scripts must be sourced"... what does that mean ?
Note: thesource
command is not available on Windows.
Other remarks:
- The minimum versions for Python and mitmproxy will now need to be maintained in quite a few places (
CONTRIBUTING
,quicktest.yml
,test.yml
, the new scripts). This feels pretty error prone... - Just as a test, I've ran the scripts on Windows:
run-tests-with-server.sh
doesn't appears to do anything (other than flash open a new shell window & close it) and exits without giving the user information about why it's not working.start-test-environment.sh
doesn't appears to do anything (other than flash open a new shell window & close it) and exits without giving the user information about why it's not working.stop-test-environment.sh
doesn't appears to do anything (other than flash open a new shell window & close it) and exits without giving the user information about why it's not working.run-phpunit.sh
runs, but opens a new shell window and closes it immediately once the test run has finished, meaning you can't read the results.
The new window is also really annoying for other reasons, like usability issues, no scrolling, window size being tiny etc
scripts/run-tests-with-server.sh
Outdated
TEST_EXIT_CODE=$? | ||
|
||
# Stop the test environment | ||
. "${SCRIPT_DIR}/stop-test-environment.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be an error as well if the test environment couldn't be stopped cleanly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to think how best to do that, let me figure something out...
Co-authored-by: Juliette <[email protected]>
d347cb7
to
e6c455d
Compare
# Set environment variables | ||
REQUESTS_HTTP_PROXY="localhost:9002" | ||
REQUESTS_HTTP_PROXY_AUTH="localhost:9003" | ||
REQUESTS_HTTP_PROXY_AUTH_USER="test" | ||
REQUESTS_HTTP_PROXY_AUTH_PASS="pass" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the changes to the Composer script to use @putenv
(in commit fb62d2b), so this be read out from env if available ?
echo "REQUESTS_TEST_HOST_HTTP=localhost:8080" | ||
echo "REQUESTS_HTTP_PROXY=localhost:9002" | ||
echo "REQUESTS_HTTP_PROXY_AUTH=localhost:9003" | ||
echo "REQUESTS_HTTP_PROXY_AUTH_USER=test" | ||
echo "REQUESTS_HTTP_PROXY_AUTH_PASS=pass" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be using the variables instead of having the values hard-coded in the echo
?
"@putenv REQUESTS_HTTP_PROXY=localhost:9002", | ||
"@putenv REQUESTS_HTTP_PROXY_AUTH=localhost:9003", | ||
"@putenv REQUESTS_HTTP_PROXY_AUTH_USER=test", | ||
"@putenv REQUESTS_HTTP_PROXY_AUTH_PASS=pass", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these be moved to a separate Composer script which is @
referenced here ? (to avoid the duplication in the coverage:withserver
script)
Refactored the test environment setup to make it more robust and user-friendly. This consolidates multiple test commands and improves process management for local development.
Fixes #819
What Changed
CONTRIBUTING.md
to explain the new setupThe main improvement is the simplified test workflow. Instead of juggling different commands for PHPUnit versions and remembering to start/stop test servers, everything is now handled through clear, single-purpose commands.
New Scripts
scripts/start-test-environment.sh
- Boots test and proxy servers - should besource
d to set environment vairablesscripts/stop-test-environment.sh
- Clean shutdown of all processesscripts/run-phpunit.sh
- Version-agnostic PHPUnit runnerUsage
Basic testing is now simpler:
For development with local test servers: