Skip to content

Update tests to close any server sockets#79

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
Merged
clue merged 1 commit into from
Oct 13, 2022
Merged

Update tests to close any server sockets #79

clue merged 1 commit into from
Oct 13, 2022

Conversation

@dinooo13
Copy link
Contributor

@dinooo13dinooo13 commented Oct 12, 2022

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

Based on reactphp/socket#283

Ref: #77

@dinooo13
Copy link
ContributorAuthor

dinooo13 commented Oct 12, 2022

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

SimonFrings
SimonFrings approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@SimonFringsSimonFrings 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.

@SimonFringsSimonFrings added this to the v1.4.0 milestone Oct 13, 2022
@SimonFringsSimonFrings requested a review from clueOctober 13, 2022 10:54
@clueclue changed the title Close open connections in testsUpdate tests to close any server socketsOct 13, 2022
clue
clue approved these changes Oct 13, 2022
View reviewed changes
Copy link
Owner

@clueclue 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! 👍

@clueclue merged commit 847aab4 into clue:masterOct 13, 2022
@dinooo13
Copy link
ContributorAuthor

dinooo13 commented Oct 13, 2022•
edited
Loading

@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 👍

@dinooo13dinooo13 deleted the closeTestConnections branch October 13, 2022 12:11
@SimonFringsSimonFrings mentioned this pull request Oct 17, 2022
Merged
@dinooo13dinooo13 mentioned this pull request Oct 21, 2022
Draft
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clueclueclue approved these changes

+1 more reviewer

@SimonFringsSimonFringsSimonFrings approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

maintenance

Projects

None yet

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@dinooo13@clue@SimonFrings