Member

1 ) Amp\ReactAdapter\Test\AdapterTest::runShouldReturnWhenNoMoreFds
Failed asserting that 0.022511959075927734 is less than 0.015.

This leaves us with the question what is a good timeout in the first place? :-)

Also refs #70

Contributor Author

@clue Could you merge this?

Member

As with any PR, this will be merged once it receives the required number of approvals 👍

I don't see my above comment addressed, have you had a chance to look into this or may I ask you to check your above link for additional error messages?

Contributor Author

No timeout is fine if execution can be arbitrarily slow. The new timeout seems to work, the old one didn't.

Member

@kelunik I'm not sure we're looking at the same thing here, as the above link and my above quote clearly say that the tests failed because the test actually took > 0.2s to execute :-)

Hence my question about what a "good" timeout value could possible be?

On a side note, the tests are not part of our public API, so we may introduce "breaking changes" at any time. As such, I wouldn't recommend relying on the test suite to test your adapter.

Contributor Author

It took more than 0.02, but the timeout is 0.06 there, as it's three times the timeout.

Tests not being part of the public API is totally fine, as the adapter's tests aren't public API either. The existing tests are the best thing available to test the adapter.

Member

Tbh I don't seen an issue raising it to 0.1 to be in the clear here

Member

Agreeing with @WyriHaximus here 👍 We've had to increase the test timeouts a number of times already because of sluggish test platform performance, while the actual times on a normal system are somewhat lower actually.

Member

I'm closing this for now as it hasn't received any input in a while and I believe this has been answered. Please come back with more details if this problem persists and we can reopen this 👍

Contributor Author

@clue No, it hasn't really been answered. We're still running with the hack of defining HHVM_VERSION.

Member

@kelunik Maybe I'm missing something obvious here, but I've just checked the previous test runs on https://travis-ci.org/reactphp/event-loop/builds and I could not find an occurrence where the test timeout was an issue for HHVM. Perhaps you can elaborate on what issue this PR aims to fix?

Member

@kelunik I get that you're having some issues in the project you've linked. However, to me it looks like you're trying to fix a problem in the wrong place. This is apparently not an issue in this library (see above link to Travis logs). This is an issue in how your project tries to run your tests.

Again, your tests currently seem to rely on the tests of this project. We may change our tests at any time without prior notice, so I explicitly want to discourage you from relying on our non-public API. If you insist on relying on this nevertheless, I would suggest just assigning $this->tickTimeout = 0.1; in your project.

Contributor Author

@clue This is an issue that you obviously already faced with HHVM. Both you and @WyriHaximus agreed that it's probably fine increasing the timeout in #92 (comment) and #92 (comment).

As said previously, those tests are the best we can do to ensure compatibility. Tests breaking at any time is entirely fine with us, we'll just adapt. $this->tickTimeout = 0.1; won't work, because the property is private. Using reflection is just as much of a hack as the current define('HHVM_VERSION').

It's a really small PR that avoids us using hacks and doesn't do any harm to your package either, so I really don't understand the resistance.

Member

Let's try it this way: I won't block this if this is accepted by the rest of the team, ping @reactphp/core, any thoughts?

Member

I don't see any reason to block this, it won't bite us at all from what I can see 👍

Member

Choose a reason for hiding this comment

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

Member

@kelunik Thank you for both your patience and persistence, let's get this in 👍

Contributor Author

Thanks!