chore: improve flaky tests (WIP) #150
Conversation
There was a problem hiding this comment.
Thanks. I left a few comments.
Furthermore, please separate the unrelated changes into separate commits. I see at least 3 separate changes:
- Fix dev extension requriements
- Modernize PHPunit API usage (this could be multiple commits or one. If it's one, please list the applied changes in the commit message)
- Increase robustness of socket-based tests
| $readData = $this->socketClient->read($numBytes); | ||
|
|
||
| self::assertEquals($testData, $readData); | ||
| } | ||
|
|
||
| public function testReadContents(): void | ||
| { | ||
| $testData = str_repeat("0123456789", mt_rand(1, 10)); | ||
| $testData = str_repeat("0123456789", random_int(1, 10)); | ||
| #$testData = str_repeat("0123456789", 10); |
There was a problem hiding this comment.
I will do after tests are stable.
| stream_copy_to_stream($connection, $connection, strlen($testData)); | ||
| self::assertEquals(strlen($testData), stream_copy_to_stream($connection, $connection, strlen($testData))); | ||
|
|
||
| usleep(300); |
There was a problem hiding this comment.
These timeouts repeat across multiple tests, it may be worth tp put them behind a descriptive constant? At least add a comment why that "random" sleep is required and how you chose the length of the timeout.
There was a problem hiding this comment.
I hope to remove this, after root cause is found. Because that depends froms system to system i guess. I was just playing around with the numbers to test it.
|
|
||
| $numBytes = $this->socketClient->write($testData); | ||
| self::assertEquals(strlen($testData), $numBytes); | ||
|
|
||
| // lower timeout for server-socket | ||
| stream_set_timeout($this->serverSocket, 0, 100); | ||
| self::assertTrue(stream_set_timeout($this->serverSocket, 0, 100)); |
There was a problem hiding this comment.
You ported a bunch of self:: to $this-> in different code-locations. Any particular reason? It probably makes sense to stick to a homogenous style.
There was a problem hiding this comment.
The PhpUnit function is not static, and that is why PhpStorm nags about this. I just want to make sure, that this is not the reason for the flaky tests. I guess both versions are correct. That was the offical answer i cheked the last time.
|
@bzikarsky Thanks for your review. I tried several things to solve the problem. It is not solved to 100% actual. I hope you can help with the root cause of the problem. After that is found and fixed, many changes of this PR could be reverted (or split in seperate PRs). Can you reproduce the flaky test locally? What system are you useing local? |
|
I'm on Linux on all my platforms. I'm open to not waste a lot of time on debugging this though. It seems not completely unlikely that there is something between PHP, the system calls and the (MacOS) kernel that requires a small waiting time before the socket becomes functional. As this only affects tests I am fine to have a small sleep and a comment on why this exists. |
|
As soon as i have some time, i will split it up in different PRs and refatore it. |
No description provided.