Refactor docker setup #339

thePanz · 2024-03-13T22:48:07Z

Remove old Dockerfiles setup
Remove docker-compose.yml
Update to PHP Alpine images
Introduce just for automation and scripting (requires: https://just.systems )

connorhu · 2024-03-14T09:12:32Z

Refactor doker setup => Refactor docker setup

mentalstring · 2024-03-19T11:55:20Z

test/unit/validator/sfValidatorFileTest.php

@@ -90,7 +90,7 @@ public function getMimeTypesFromCategory($category)
 $v = new testValidatorFile();
 $t->is($v->guessFromFileBinary($tmpDir.'/test.txt'), 'text/plain', '->guessFromFileBinary() guesses the type of a given file');
 $t->is($v->guessFromFileBinary($tmpDir.'/foo.txt'), null, '->guessFromFileBinary() returns null if the file type is not guessable');
-$t->like($v->guessFromFileBinary('/bin/ls'), (PHP_OS != 'Darwin') ? '/^application\/x-(pie-executable|executable|sharedlib)$/' : '/^application/octet-stream$/', '->guessFromFileBinary() returns correct type if file is guessable');
+$t->is($v->guessFromFileBinary('/usr/bin/file'), 'application/x-pie-executable', '->guessFromFileBinary() returns correct type if file is guessable');


Won't this make the check pass only under some OS/distros? In other words, do we want to tests to only work under the docker setup?

I guess so, this is to simplify the tests and the pipelines IMO.
Having a reliable docker setup would mean that we can rely on a single and common OS where tests are run.

Otherwise, to be consistent, we would need to have a pipeline testing that part for different OSes.

WDYT?

It's definitely great that there's a consistent setup to run tests in, specially one that it can be used to easily run them against multiple php versions without having to maintain that mess locally – it's great for that and I'm all for it.

At the same time, it seems a pity that we opt to drop the possibility of easily running (by easy I mean having them pass) the whole test suite in different environments (be it macos, windows or some random linux distro) and adding the requirement to install/start/use docker to run the test suite.

Also, by making it less easy to run the tests in multiple contexts (e.g. by developers) we also lose some visibility into scenarios where the code&tests may be failing outside of the static docker setup which we may end up finding only when already running the code in some production server, where the set up is always going to be different than the docker one.

Otherwise, to be consistent, we would need to have a pipeline testing that part for different OSes.

I don't think we need to have CI under different OS. That line was already updated a few times over the years to account for some OS specificities that different developers bumped into. Isn't it possible to tweak it to include whatever alpine does differently, and allow the test suite to continue to run (read pass) without docker?

PS: One way to at least avoid different distro layouts might be to test the function against PHP_BINARY which should always return an executable.

@mentalstring I like your suggestion to use PHP_BINARY! nice trick! I applied the change and tests passed 👍

Actually, another idea to get this to run across different systems, hopefully definitely.

I don't see a good reason to be doing this test against a random executable file which may return different things in different systems – the binary in the name of the function refers to using the system file util, not that it only tests binary files.

Hence, instead of PHP_BINARY, test it against __FILE__ and expect 'text/x-php' – from a quick test, this works both in linux and macos (guessFromFileBinary() doesn't work in windows).

connorhu · 2024-03-19T14:31:43Z

Please update the README.md file too if anything changes.

thePanz · 2024-03-20T17:14:44Z

@connorhu @mentalstring I would need some help here: I did not setup any MySQL server and the tests are all green. Something is weird IMO

connorhu · 2024-03-20T17:19:41Z

@thePanz Anno I started looking at the possibility of psr-4 migration and even with obvious problems the test was green with lime. Then I began to port the test system to phpunit because I found lime to be totally unreliable.
On topic: how can I help? Should I look at your branch?

mentalstring · 2024-03-20T22:50:39Z

@connorhu @mentalstring I would need some help here: I did not setup any MySQL server and the tests are all green. Something is weird IMO

Not sure if this is what you are referring to, but I think only unit/storage/sfMySQLStorageTest.php would use MySQL and it skips the test if the mysql extension is not installed. unit/storage/sfMySQLiStorageTest.php does the same if sqlite is not installed.

Currently, running tests with the symfony:test hides skipped tests behind a 'ok' which is how sfAPCCacheTest wasn't running on the CI for a long while due to test being skipped.

It also hides a bunch notices/warnings (e.g. try php unit/validator/sfValidatorSchemaTest.php). I think there is some work on #302 that maybe helps with this, but I didn't look closely.

mentalstring · 2024-03-20T22:52:15Z

Speaking of tests and refactoring the docker setup, I think test/bin/test is no longer needed?

thePanz marked this pull request as draft March 13, 2024 22:48

thePanz force-pushed the update-doker-setup branch 2 times, most recently from 3d949d9 to 1b00de4 Compare March 13, 2024 23:27

thePanz requested review from thirsch and connorhu March 13, 2024 23:29

thePanz changed the title ~~Refactor doker setup~~ Refactor docker setup Mar 14, 2024

thePanz force-pushed the update-doker-setup branch 2 times, most recently from efa71ef to 88710d0 Compare March 19, 2024 10:52

mentalstring reviewed Mar 19, 2024

View reviewed changes

thePanz force-pushed the update-doker-setup branch 2 times, most recently from a6463df to 23cd8a0 Compare March 19, 2024 12:59

thePanz force-pushed the update-doker-setup branch from 23cd8a0 to 2221e70 Compare March 20, 2024 17:12

thePanz added 4 commits March 20, 2024 18:23

Refactor(docker): initial refactor of Docker setup

f10af5a

Docker: remove legacy docker-compose.yml file

fb222b7

Fix(docker): Fix tests for file-type checks on new docker alpine images

a91b899

WIP: add MySQL to docker setup

d3d624b

thePanz force-pushed the update-doker-setup branch from 2221e70 to d3d624b Compare March 20, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor docker setup #339

Refactor docker setup #339

thePanz commented Mar 13, 2024 •

edited

Loading

connorhu commented Mar 14, 2024

mentalstring Mar 19, 2024

thePanz Mar 19, 2024

mentalstring Mar 19, 2024

mentalstring Mar 19, 2024

thePanz Mar 20, 2024 •

edited

Loading

mentalstring Mar 20, 2024 •

edited

Loading

connorhu commented Mar 19, 2024

thePanz commented Mar 20, 2024

connorhu commented Mar 20, 2024

mentalstring commented Mar 20, 2024

mentalstring commented Mar 20, 2024

Refactor docker setup #339

Are you sure you want to change the base?

Refactor docker setup #339

Conversation

thePanz commented Mar 13, 2024 • edited Loading

connorhu commented Mar 14, 2024

mentalstring Mar 19, 2024

Choose a reason for hiding this comment

thePanz Mar 19, 2024

Choose a reason for hiding this comment

mentalstring Mar 19, 2024

Choose a reason for hiding this comment

mentalstring Mar 19, 2024

Choose a reason for hiding this comment

thePanz Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

mentalstring Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

connorhu commented Mar 19, 2024

thePanz commented Mar 20, 2024

connorhu commented Mar 20, 2024

mentalstring commented Mar 20, 2024

mentalstring commented Mar 20, 2024

thePanz commented Mar 13, 2024 •

edited

Loading

thePanz Mar 20, 2024 •

edited

Loading

mentalstring Mar 20, 2024 •

edited

Loading