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.

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

feat: Add wait_for_* functions in NetworkUtil for performing check on port, host, http_code #216

Merged
merged 11 commits into from
Jan 10, 2024

Conversation

TheoD02
Copy link
Contributor

@TheoD02 TheoD02 commented Jan 4, 2024

Hello all! :)

The PR responds to the demand of the issue #207

Here is the first draft of what is possible 🙂

For all methods, a $quiet = false and $throw = false parameter is available to permit not showing anything or throwing instead of returning a boolean on check success/fail.

I have introduced a wait_for_port that can be useful to wait for port X on host X.

Untitled

wait_for_url to wait for a certain URL to be accessible.

Untitled (1)

wait_for_http_status to wait for a certain URL with a specific status code (default to 200).
Additional idea not in PR: A callable can be added to check if some content is present in the request to continue, for example.
(Maybe duplicate with the first wait_for_url. Is it useful to keep both?)

Untitled (2)

wait_for_docker_container is a helper that can be used only when Docker is present. It can autostart the container and use both Docker or Docker Compose approach, and auto-check the binded ports or those provided by default.

Untitled (3)

Ports can be auto-detected by Docker inspect only when portsToCheck is equal to [].

Untitled (4)

I added one callback additionalCheck that is useful when, for example, with MySQL, the container is UP, the port is accessible, but it cannot do anything for a certain time until the container is fully ready. For this case, I run a MySQL Query to check until it responds to go forward.

Example for the MySQL case:

The container started, and 3306 seems to be accessible, but not really fully available.

Untitled (5)

When the available check is present, the check is performed before doing anything in the MySQL container.

Untitled (6)

That is a case I already have in a project. I think I'm not alone 😀

Please tell me your feedback and suggestions :)

I don't really know how I can fully test this for now, but I will work on it, at least for the non-Docker wait_for part.

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

This is a good start 👍🏼

The docker part add too much complexity IMHO. Could you split the PR in two? one without docker, and one with docker?

Like that we could merge the first one ASAP

src/NetworkUtil.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Outdated Show resolved Hide resolved
src/functions.php Outdated Show resolved Hide resolved
@TheoD02
Copy link
Contributor Author

TheoD02 commented Jan 5, 2024

This is a good start 👍🏼

The docker part add too much complexity IMHO. Could you split the PR in two? one without docker, and one with docker?

Like that we could merge the first one ASAP

Hey, thanks for your feedback 😃

I deleted the docker part, I'll do the PR later 😄

I've corrected the main feedback, I'm waiting to get home to finalize the last fixes :) (phpstan can't run in codespace 🥲)

I'll refactor the code to add the wait_for method and use on it in the other wait_for_* methods.

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Except one little comment, the code is OKAY

Could you also

  • Add a note in the CHANGELOG.md
  • Add some documentation

Thanks

src/NetworkUtil.php Outdated Show resolved Hide resolved
@pyrech
Copy link
Member

pyrech commented Jan 5, 2024

I really like your PR, thanks for your contribution @TheoD02 🙌

I only worry about the class NetworkUtil you introduced because every others helpers Castor provide are functions and not class methods. So I would prefer your new helpers to be functions too, alongside the ones already existing.

@TheoD02
Copy link
Contributor Author

TheoD02 commented Jan 6, 2024

Re,

I fixed the latest feedback

I added the doc, changelog, tests :)

Can you review the NetworkUtil class again, I've made quite a few changes to it ?

I've added a generic wait_for method that will allow us to do a wait for other cases, and the PR existant methods are based on this method.

@TheoD02 TheoD02 changed the title feat: Add wait_for_* functions in NetworkUtil for performing check on port, host, http_code or docker container feat: Add wait_for_* functions in NetworkUtil for performing check on port, host, http_code Jan 6, 2024
@TheoD02
Copy link
Contributor Author

TheoD02 commented Jan 8, 2024

Is it normal that simple-phpunit to update all dependencies regardless of the version restriction?
composer upgrade nikic/php-parser do the upgrade to 4.18.0, but simple-phpunit to the latest 5.0 released yesterday ^^

image

This make the tests fail :
image

Running test through phpunit/phpunit work fine

@lyrixx
Copy link
Member

lyrixx commented Jan 9, 2024

yes, there is a bug ATM in symfony :/

Could you try to add <server name="SYMFONY_PHPUNIT_REQUIRE" value="nikic/php-parser:^4" /> in phpunit.xml.dist ?

thanks

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

I let some comments about the arguments // options.

Otherwise, It's cool 👏🏼 thanks

doc/15-wait-for.md Outdated Show resolved Hide resolved
src/Exception/WaitForInvalidCallbackCheckException.php Outdated Show resolved Hide resolved
src/Exception/WaitForTimeoutReachedException.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Outdated Show resolved Hide resolved
src/NetworkUtil.php Show resolved Hide resolved
src/NetworkUtil.php Show resolved Hide resolved
Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Yeah ! This is very nice. Thanks !

LGTM

doc/15-wait-for.md Outdated Show resolved Hide resolved
Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

Nice work 🙂

@pyrech pyrech merged commit bfd7533 into jolicode:main Jan 10, 2024
3 checks passed
@lyrixx
Copy link
Member

lyrixx commented Jan 10, 2024

@TheoD02 Thanks for the initial work, I cleaned a bit your implementation in #223 and simplified few bits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants