-
Notifications
You must be signed in to change notification settings - Fork 17
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
Run tests in isolation (using Docker) #194
Conversation
34bff62
to
553387d
Compare
This makes the tests less dependent on the developer’s system. and FINALLY! Fixes the ICU versioning problem.
HAHAHA IT WORKED! |
pdo_pgsql \ | ||
pgsql \ | ||
bcmath \ | ||
intl \ |
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.
xdebug?
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.
It depends, I'm not sure this is required for the qa tools 🤔 maybe for the main dev image it's handy for remote debugging and coverage (although php-dbg is more recommended now).
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.
TIL php-dbg can collect coverage data. 👍
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.
BTW @jakzal confirmed in jakzal/phpqa#18 (comment) you can already use phpdbg from his image so this can be ignored.
Makefile
Outdated
test-full: | ||
export SYMFONY_DEPRECATIONS_HELPER=strict | ||
|
||
# These tasks should be executed in paralel. |
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.
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.
Yes, I know. I already tried this in the past with the script symfony/symfony uses but failed to make it work then 😃
If you can help with this that would be great 👍
FYI I am away from ⌨️ on Wednesday and Thursday.
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.
Just tried it, don't think you actually need to mess with parallel here, DB specific tests here are 7sec on my system in total while the regular part is ~30s.
0438fee
to
b99ea9c
Compare
IMO once you merge this, you should consider tagging to see what happens (with the mono-repo and all). |
The splitting of commits is not automated yet 😛 I will try to contact Fabien if he can set this up for RollerworksSearch 👍 |
Don't know what exactly he does to set it up, but the I took the scripts from enqueue-dev. |
Makefile
Outdated
|
||
dist: cs-full phpstan test-full | ||
dist: install cs-full phpstan test-full |
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.
Install probably not needed here, especially since there's no composer.phar
on all systems (mine, for example).
Makefile
Outdated
ci: cs-full-check phpstan test-full | ||
|
||
install: | ||
composer.phar install --no-progress --no-interaction --no-suggest --optimize-autoloader --ansi |
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.
Can we use composer from Docker or something? Hardcoding composer.phar
here doesn't seem right
Makefile
Outdated
test-full: | ||
export SYMFONY_DEPRECATIONS_HELPER=strict | ||
|
||
# These tasks should be executed in paralel. |
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.
Just tried it, don't think you actually need to mess with parallel here, DB specific tests here are 7sec on my system in total while the regular part is ~30s.
Makefile
Outdated
phpdbg -qrr vendor/bin/phpunit --verbose --configuration travis/mysql.travis.xml --coverage-php build/cov/coverage-phpunit-mysql.cov | ||
phpdbg -qrr phpcov.phar merge --clover build/logs/clover.xml build/cov | ||
|
||
test-isolated: docker-up |
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'd go with simpler labels for targets you wish developers to use directly. For example, if you'd document the development workflow, you'd want to use simpler labels for human use, sort of like public methods vs private methods. :)
Makefile
Outdated
test-with-coverage: | ||
export SYMFONY_DEPRECATIONS_HELPER=strict | ||
|
||
curl -Ls https://phar.phpunit.de/phpcov.phar > phpcov.phar |
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.
Actually not needed as the qa image contains it.
docker-compose.yaml
Outdated
volumes: | ||
- pg-data:/var/lib/postgresql/data:rw | ||
ports: | ||
- "5432:5433" |
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.
Is this an error? Exporting a port on a off-by-one port?
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.
This is only used for the host. Some people (including me) have a local instance running.
The docker instances still use the original port.
In fact I have been pulling out my hairs why the hell I can't connect from the host to this server instance 💀 or MariaDB for that matter. Elasticsearch connects without any issues.
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.
Yeah, I thought so. If you're only changing the port to avoid collisions, I'd go with something way off instead of just off by one (because it might be considered a typo).
For example, prefixing all ports with 5?
MySQL: 53306
PgSQL: 55432
Elastic: 59200
etc.
That way you see from a mile away it's very different, there's a really low chance of collisions and you can actually document the difference easily. :)
I tagged the sub-splits manually for now. I am working on a more permanent solution 👍 |
Good news HubKit now allows to split a monolith repository while merging, and... can synchronize a release for all split repositories! So finally no more outdated releases 🎉 I am continuing work on RollerworksSearch 👍 |
Very nice, LGTM! 🏁 |
Ah shoot I forgot to remove the WIP commits 😫 that's what I get for working after 7 PM 🤦♂️ |
volumes: | ||
- esdata:/usr/share/elasticsearch/data | ||
ports: | ||
- "9200:9200" |
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.
Note, this was not prefixed with 5. :( (using a prefix here)
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.
For some reasons this broke the tests 😐 (using a prefix for this port).
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 know, tried to change it. 😢 Will submit a PR to fix it once I figure out what it is.
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.
It's over 9200 😂 https://stackoverflow.com/questions/113224/what-is-the-largest-tcp-ip-network-port-number-allowable-for-ipv4
The port number seems to high.
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.
But... 59200 < 65535? That's why I proposed 5 as a prefix, and not 6, for example.
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.
Either specify both ports (HOST:CONTAINER), or just the container port (a random host port will be chosen).
See #200
This makes the tests less dependent on the developer’s system. and FINALLY! Fixes the ICU versioning problem.
Note: this is a proof of concept, I am open to any suggestions 👍