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.

Sign up for GitHub

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

Jump to bottom

Refactor docker setup #339

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Refactor docker setup #339

wants to merge 4 commits into from

Conversation

thePanz
Copy link
Member

@thePanz thePanz commented Mar 13, 2024 •
edited
Loading

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

@thePanz thePanz marked this pull request as draft March 13, 2024 22:48
@thePanz thePanz force-pushed the update-doker-setup branch 2 times, most recently from 3d949d9 to 1b00de4 Compare March 13, 2024 23:27
@connorhu
Copy link
Collaborator

Refactor doker setup => Refactor docker setup

@thePanz thePanz changed the title Refactor doker setup Refactor docker setup Mar 14, 2024
@thePanz thePanz force-pushed the update-doker-setup branch 2 times, most recently from efa71ef to 88710d0 Compare March 19, 2024 10:52
mentalstring
mentalstring reviewed Mar 19, 2024
View reviewed changes
test/unit/validator/sfValidatorFileTest.php Outdated
@@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@thePanz thePanz Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mentalstring mentalstring Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

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

@thePanz
Copy link
Member Author

thePanz commented Mar 20, 2024

@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
Copy link
Collaborator

@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
Copy link
Contributor

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mentalstring mentalstring mentalstring left review comments

@thirsch thirsch Awaiting requested review from thirsch

@connorhu connorhu Awaiting requested review from connorhu

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thePanz @connorhu @mentalstring