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

Update tests to close any server sockets #79

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

dinooo13
Copy link
Contributor

Close open connections in tests to prevent them from looping in the async update.

Based on reactphp/socket#283

Ref: #77

@dinooo13
Copy link
Contributor Author

I think this should do it for the async update. What do you think?

Copy link
Contributor

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

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

@dinooo13 I tested this in my private fork together with your changes in #77 and the tests for PHP 8.1 and PHP 8.2 don't get stuck anymore 👍

I actually thought that this pull request would involve some bigger changes, that's why I suggested to open up an additional PR. I think this is good as it is, but if clue/reactphp-socks#110 involves the same amount of changes we can just add a second commit in there.

@SimonFrings SimonFrings added this to the v1.4.0 milestone Oct 13, 2022
@SimonFrings SimonFrings requested a review from clue October 13, 2022 10:54
@clue clue changed the title Close open connections in tests Update tests to close any server sockets Oct 13, 2022
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@dinooo13 Thank you for looking into this, changes LGTM! 👍

@clue clue merged commit 847aab4 into clue:master Oct 13, 2022
@dinooo13
Copy link
Contributor Author

dinooo13 commented Oct 13, 2022

@dinooo13 I tested this in my private fork together with your changes in #77 and the tests for PHP 8.1 and PHP 8.2 don't get stuck anymore 👍

I actually thought that this pull request would involve some bigger changes, that's why I suggested to open up an additional PR. I think this is good as it is, but if clue/reactphp-socks#110 involves the same amount of changes we can just add a second commit in there.

In clue/reactphp-socks#110 it's quite a bit more so I think the additional PR is the way to go 👍

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

Successfully merging this pull request may close these issues.

3 participants