promises-aplus / promises-tests

Compliances tests for Promises/A+
Other
943 stars 107 forks source link

Timeouts inside tests #53

Closed mcharytoniuk closed 10 years ago

mcharytoniuk commented 10 years ago

Is it necessary to keep seemingly random timeouts like these inside tests?

https://github.com/promises-aplus/promises-tests/blob/master/lib/tests/2.2.6.js#L131 https://github.com/promises-aplus/promises-tests/blob/master/lib/tests/2.3.2.js#L51 https://github.com/promises-aplus/promises-tests/blob/master/lib/tests/2.3.2.js#L112

In most cases there is no clear reason for them and they make tests run at least 12 seconds. This time could be substantially reduced.

wizardwerdna commented 10 years ago

The timeouts run in ms, not seconds. And yes some timeout is necessary, since callbacks are ALWAYS run asynchronously (in future turns) per the specification. Thus, the test cannot confirm things, such as as the order of callback, without some kind of timeout sufficient that all the queues are flushed.

On Thu, Mar 6, 2014 at 1:34 AM, Mateusz Charytoniuk < notifications@github.com> wrote:

Is it necessary to keep seemingly random timeouts like these inside tests?

https://github.com/promises-aplus/promises-tests/blob/master/lib/tests/2.2.6.js#L131

https://github.com/promises-aplus/promises-tests/blob/master/lib/tests/2.3.2.js#L51

https://github.com/promises-aplus/promises-tests/blob/master/lib/tests/2.3.2.js#L112

In most cases there is no clear reason for them and they make tests run at least 12 seconds. This time could be substantially reduced.

Reply to this email directly or view it on GitHubhttps://github.com/promises-aplus/promises-tests/issues/53 .

mcharytoniuk commented 10 years ago

But in total they run about 12 seconds. If asynchrony is the only reason then why setTimeout(fun, 0); is not enough? This is strange - what if flushing some internal queue in some implementation takes sometimes 16ms and sometimes 14ms - depending on CPU load or other random factors (then tests sometimes fail and sometimes not - it means that they are not reliable)? Why 15ms and 50ms and not 14ms and 39ms, etc, etc... ?

domenic commented 10 years ago

Indeed, the timeouts are arbitrary. If an implementation reports a false negative due to too-short timeouts, we can lengthen them.

wizardwerdna commented 10 years ago

This has never reached a pain point in the testing, in my view. An alternative would be to inject an enqueue operation to the library and test with a mock/flush operation, which would permit testing and not require any delays.

On Thu, Mar 6, 2014 at 6:53 PM, Domenic Denicola notifications@github.comwrote:

Indeed, the timeouts are arbitrary. If an implementation reports a false negative due to too-short timeouts, we can lengthen them.

Reply to this email directly or view it on GitHubhttps://github.com/promises-aplus/promises-tests/issues/53#issuecomment-36963395 .

karfau commented 10 years ago

I also thought about this topic, and i really liked the idea. But then to be able to fullfill the testsuite the library would have to support something that has nothing to to with the actual specification.