2 seconds default time limit for tests

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

2 seconds default time limit for tests

Denis Kudriashov
Hi.

In Pharo 6 we introduced time limit for tests. Currently it is set to 1 minute by default. And concrete test cases or single tests can redefine it with suitable value. There is also setting to change default limit globally which should help to not adopt external projects immediately if current limit is bad for them.

There is issue 19105 to make limit really small. It will set default limit for 2 seconds. Slow tests are already marked with bigger value. So it is safe for integration

Here I ask for your agreement with this change. Or disagreement if there are good reasons to not do this. 

I hope following story will convince you that default time limit should be small:

I worked on tests for new project and I got hanging tests. These tests were waiting for some event which not happens because of bugs. They will be failed after 1 minutes due to timeout.

But when you work on code you of course do not want to wait 1 minute. So it always forces manual interrupt of running tests. But then you could not run full test suite to see all problem tests. And also it is not really easy to detect current running test after interrupt. You need analyze stack in debugger and if you just close debugger information is lost.

So to fix it I change default timeout in each of my test cases.
If you will work on similar code you will need same approach to comfortably detect and fix "synchronization" bugs. 

So this was my story. I am sure default behaviour with small time limit will make SUnit much better.
Common case is that unit tests are supposed to be fast. Users expect it by default. And if something is going wrong fast tests should fail fast. 
When user wrote slow test it is expected to know that test is slow. And for such rare cases user can mark slow tests explicitly. But SUnit should not expect tests to be slow by default.

I think it is really good improvement for Pharo 6. It makes TDD in Pharo better.

Best regards,
Denis
Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Guillermo Polito
I vote for not introducing during code-freeze.

We can discuss it for Pharo 7, I'm not against actually. But I have some concerns:

- How does it work when we are debugging?
I mean, imagine I'm running a test, a debugger is open, and I start working on it. How do these timeout currently work in this scenario? I imagine that the test runner should detect you entered into a "I'm debugging" mode and should not kill your test, but this is not clear to me how this works right now.

- Also, how can we better document these things? Because I was bitten a couple of times by this timeouts and the "I kill your process but I do not tell you", and it was not nice because it was completely silent or obscure.

Guille

On Thu, Feb 16, 2017 at 1:33 PM, Denis Kudriashov <[hidden email]> wrote:
Hi.

In Pharo 6 we introduced time limit for tests. Currently it is set to 1 minute by default. And concrete test cases or single tests can redefine it with suitable value. There is also setting to change default limit globally which should help to not adopt external projects immediately if current limit is bad for them.

There is issue 19105 to make limit really small. It will set default limit for 2 seconds. Slow tests are already marked with bigger value. So it is safe for integration

Here I ask for your agreement with this change. Or disagreement if there are good reasons to not do this. 

I hope following story will convince you that default time limit should be small:

I worked on tests for new project and I got hanging tests. These tests were waiting for some event which not happens because of bugs. They will be failed after 1 minutes due to timeout.

But when you work on code you of course do not want to wait 1 minute. So it always forces manual interrupt of running tests. But then you could not run full test suite to see all problem tests. And also it is not really easy to detect current running test after interrupt. You need analyze stack in debugger and if you just close debugger information is lost.

So to fix it I change default timeout in each of my test cases.
If you will work on similar code you will need same approach to comfortably detect and fix "synchronization" bugs. 

So this was my story. I am sure default behaviour with small time limit will make SUnit much better.
Common case is that unit tests are supposed to be fast. Users expect it by default. And if something is going wrong fast tests should fail fast. 
When user wrote slow test it is expected to know that test is slow. And for such rare cases user can mark slow tests explicitly. But SUnit should not expect tests to be slow by default.

I think it is really good improvement for Pharo 6. It makes TDD in Pharo better.

Best regards,
Denis

Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

EstebanLM

On 16 Feb 2017, at 13:49, Guillermo Polito <[hidden email]> wrote:

I vote for not introducing during code-freeze.

+1: not in code freeze.


We can discuss it for Pharo 7, I'm not against actually. But I have some concerns:

- How does it work when we are debugging?
I mean, imagine I'm running a test, a debugger is open, and I start working on it. How do these timeout currently work in this scenario? I imagine that the test runner should detect you entered into a "I'm debugging" mode and should not kill your test, but this is not clear to me how this works right now.

- Also, how can we better document these things? Because I was bitten a couple of times by this timeouts and the "I kill your process but I do not tell you", and it was not nice because it was completely silent or obscure.

Guille

On Thu, Feb 16, 2017 at 1:33 PM, Denis Kudriashov <[hidden email]> wrote:
Hi.

In Pharo 6 we introduced time limit for tests. Currently it is set to 1 minute by default. And concrete test cases or single tests can redefine it with suitable value. There is also setting to change default limit globally which should help to not adopt external projects immediately if current limit is bad for them.

There is issue 19105 to make limit really small. It will set default limit for 2 seconds. Slow tests are already marked with bigger value. So it is safe for integration

Here I ask for your agreement with this change. Or disagreement if there are good reasons to not do this. 

I hope following story will convince you that default time limit should be small:

I worked on tests for new project and I got hanging tests. These tests were waiting for some event which not happens because of bugs. They will be failed after 1 minutes due to timeout.

But when you work on code you of course do not want to wait 1 minute. So it always forces manual interrupt of running tests. But then you could not run full test suite to see all problem tests. And also it is not really easy to detect current running test after interrupt. You need analyze stack in debugger and if you just close debugger information is lost.

So to fix it I change default timeout in each of my test cases.
If you will work on similar code you will need same approach to comfortably detect and fix "synchronization" bugs. 

So this was my story. I am sure default behaviour with small time limit will make SUnit much better.
Common case is that unit tests are supposed to be fast. Users expect it by default. And if something is going wrong fast tests should fail fast. 
When user wrote slow test it is expected to know that test is slow. And for such rare cases user can mark slow tests explicitly. But SUnit should not expect tests to be slow by default.

I think it is really good improvement for Pharo 6. It makes TDD in Pharo better.

Best regards,
Denis


Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Sven Van Caekenberghe-2
But we could integrate the patch (his work marking slow tests) and the code, and then disable the enforcing mechanism. That will allow us to re-able it quickly later on.

> On 16 Feb 2017, at 14:23, Esteban Lorenzano <[hidden email]> wrote:
>
>
>> On 16 Feb 2017, at 13:49, Guillermo Polito <[hidden email]> wrote:
>>
>> I vote for not introducing during code-freeze.
>
> +1: not in code freeze.
>
>>
>> We can discuss it for Pharo 7, I'm not against actually. But I have some concerns:
>>
>> - How does it work when we are debugging?
>> I mean, imagine I'm running a test, a debugger is open, and I start working on it. How do these timeout currently work in this scenario? I imagine that the test runner should detect you entered into a "I'm debugging" mode and should not kill your test, but this is not clear to me how this works right now.
>>
>> - Also, how can we better document these things? Because I was bitten a couple of times by this timeouts and the "I kill your process but I do not tell you", and it was not nice because it was completely silent or obscure.
>>
>> Guille
>>
>> On Thu, Feb 16, 2017 at 1:33 PM, Denis Kudriashov <[hidden email]> wrote:
>> Hi.
>>
>> In Pharo 6 we introduced time limit for tests. Currently it is set to 1 minute by default. And concrete test cases or single tests can redefine it with suitable value. There is also setting to change default limit globally which should help to not adopt external projects immediately if current limit is bad for them.
>>
>> There is issue 19105 to make limit really small. It will set default limit for 2 seconds. Slow tests are already marked with bigger value. So it is safe for integration
>>
>> Here I ask for your agreement with this change. Or disagreement if there are good reasons to not do this.
>>
>> I hope following story will convince you that default time limit should be small:
>>
>> I worked on tests for new project and I got hanging tests. These tests were waiting for some event which not happens because of bugs. They will be failed after 1 minutes due to timeout.
>>
>> But when you work on code you of course do not want to wait 1 minute. So it always forces manual interrupt of running tests. But then you could not run full test suite to see all problem tests. And also it is not really easy to detect current running test after interrupt. You need analyze stack in debugger and if you just close debugger information is lost.
>>
>> So to fix it I change default timeout in each of my test cases.
>> If you will work on similar code you will need same approach to comfortably detect and fix "synchronization" bugs.
>>
>> So this was my story. I am sure default behaviour with small time limit will make SUnit much better.
>> Common case is that unit tests are supposed to be fast. Users expect it by default. And if something is going wrong fast tests should fail fast.
>> When user wrote slow test it is expected to know that test is slow. And for such rare cases user can mark slow tests explicitly. But SUnit should not expect tests to be slow by default.
>>
>> I think it is really good improvement for Pharo 6. It makes TDD in Pharo better.
>>
>> Best regards,
>> Denis
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Denis Kudriashov
In reply to this post by Guillermo Polito

2017-02-16 13:49 GMT+01:00 Guillermo Polito <[hidden email]>:
I vote for not introducing during code-freeze.

I understand it but these behaviour was introduced in Pharo 6. And It is already used quite long time. So it could be considered as small improvements to already integrated feature. 
 

We can discuss it for Pharo 7, I'm not against actually. But I have some concerns:

- How does it work when we are debugging?
I mean, imagine I'm running a test, a debugger is open, and I start working on it. How do these timeout currently work in this scenario? I imagine that the test runner should detect you entered into a "I'm debugging" mode and should not kill your test, but this

It solved very easily. "Watchdog process" just checks that test process isSuspended or not. If it is not suspended timeout is signalled. Test process is suspended only when debugged.
 
is not clear to me how this works right now.

- Also, how can we better document these things? Because I was bitten a couple of times by this timeouts and the "I kill your process but I do not tell you", and it was not nice because it was completely silent or obscure.

Things which you show me I improved. Last days I fix Christophe issues which was really important. Anyway "fork management in tests" is not related to time limit of tests. And of course it is not supposed to make troubles but instead it should make life easy :)

Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Denis Kudriashov
In reply to this post by Sven Van Caekenberghe-2

2017-02-16 14:26 GMT+01:00 Sven Van Caekenberghe <[hidden email]>:
But we could integrate the patch (his work marking slow tests) and the code, and then disable the enforcing mechanism. That will allow us to re-able it quickly later on.

It is already in image. My patch only enable small time limit. 
Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Sven Van Caekenberghe-2

> On 16 Feb 2017, at 14:32, Denis Kudriashov <[hidden email]> wrote:
>
>
> 2017-02-16 14:26 GMT+01:00 Sven Van Caekenberghe <[hidden email]>:
> But we could integrate the patch (his work marking slow tests) and the code, and then disable the enforcing mechanism. That will allow us to re-able it quickly later on.
>
> It is already in image. My patch only enable small time limit.

But did you not mark more tests with custom timeouts ?
Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Denis Kudriashov

2017-02-16 14:35 GMT+01:00 Sven Van Caekenberghe <[hidden email]>:
But did you not mark more tests with custom timeouts ?

Yes, couple test cases. And actually all slow tests (from previous fixes too) are marked with 10 seconds. Current default 1 minute is to not affect possible external projects. But I think Pharo 6 could force them to adopt. And global setting makes it painless. 
Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Guillermo Polito
In reply to this post by Denis Kudriashov


On Thu, Feb 16, 2017 at 2:31 PM, Denis Kudriashov <[hidden email]> wrote:

2017-02-16 13:49 GMT+01:00 Guillermo Polito <[hidden email]>:
I vote for not introducing during code-freeze.

I understand it but these behaviour was introduced in Pharo 6. And It is already used quite long time. So it could be considered as small improvements to already integrated feature. 
 

We can discuss it for Pharo 7, I'm not against actually. But I have some concerns:

- How does it work when we are debugging?
I mean, imagine I'm running a test, a debugger is open, and I start working on it. How do these timeout currently work in this scenario? I imagine that the test runner should detect you entered into a "I'm debugging" mode and should not kill your test, but this

It solved very easily. "Watchdog process" just checks that test process isSuspended or not. If it is not suspended timeout is signalled. Test process is suspended only when debugged.

Ok. Is something like this in a class comment? If not, we could add it, because it will get lost in the mails.
 
 
is not clear to me how this works right now.

- Also, how can we better document these things? Because I was bitten a couple of times by this timeouts and the "I kill your process but I do not tell you", and it was not nice because it was completely silent or obscure.

Things which you show me I improved. Last days I fix Christophe issues which was really important. Anyway "fork management in tests" is not related to time limit of tests. And of course it is not supposed to make troubles but instead it should make life easy :)

I know, I'm not pleading for improvements :). Just talking about documenttion here. 

Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Ben Coman
In reply to this post by Denis Kudriashov
On Thu, Feb 16, 2017 at 9:32 PM, Denis Kudriashov <[hidden email]> wrote:
>
> 2017-02-16 14:26 GMT+01:00 Sven Van Caekenberghe <[hidden email]>:
>>
>> But we could integrate the patch (his work marking slow tests) and the
>> code, and then disable the enforcing mechanism. That will allow us to
>> re-able it quickly later on.
>
>
> It is already in image. My patch only enable small time limit.

But this could conceivably cause more  erroneous failures by the CI
monkey, which slows down progress up to the Release.

+1 for doing it after the Release.

Now what would be interesting (though maybe not feasible) would be
recording how long each test takes, maybe sending to a central server,
to do maybe do statistics on them.  Maybe faster running tests could
be scheduled first.  Or we may find some tests have multiple modes to
optimise.  Or which tests fail the timeout more often - to help set
the timeout.  Perhaps the timeout needs to scale for different
environments - like running of a RPi rather than a beast desktop.

cheers -ben

Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Denis Kudriashov
In reply to this post by Guillermo Polito

2017-02-16 14:46 GMT+01:00 Guillermo Polito <[hidden email]>:

It solved very easily. "Watchdog process" just checks that test process isSuspended or not. If it is not suspended timeout is signalled. Test process is suspended only when debugged.

Ok. Is something like this in a class comment? If not, we could add it, because it will get lost in the mails.

I added comment to watch dog process loop. 
Generally I think this question should not be raised because what you ask is the way how it should work. And if it would be not like this it would be considered as bug.
Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Denis Kudriashov
In reply to this post by Ben Coman

2017-02-16 14:59 GMT+01:00 Ben Coman <[hidden email]>:
But this could conceivably cause more  erroneous failures by the CI
monkey, which slows down progress up to the Release.

I would not wrote this mail without Monkey :). With "2 seconds" Monkey was happy.  
 

+1 for doing it after the Release.

Now what would be interesting (though maybe not feasible) would be
recording how long each test takes, maybe sending to a central server,
to do maybe do statistics on them.  Maybe faster running tests could
be scheduled first.  Or we may find some tests have multiple modes to
optimise.  Or which tests fail the timeout more often - to help set
the timeout.  Perhaps the timeout needs to scale for different
environments - like running of a RPi rather than a beast desktop.

It can be improved later. Now we do not have CI for RPi.

Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Denis Kudriashov
In reply to this post by Denis Kudriashov
I moved issue to Pharo 7

2017-02-16 13:33 GMT+01:00 Denis Kudriashov <[hidden email]>:
Hi.

In Pharo 6 we introduced time limit for tests. Currently it is set to 1 minute by default. And concrete test cases or single tests can redefine it with suitable value. There is also setting to change default limit globally which should help to not adopt external projects immediately if current limit is bad for them.

There is issue 19105 to make limit really small. It will set default limit for 2 seconds. Slow tests are already marked with bigger value. So it is safe for integration

Here I ask for your agreement with this change. Or disagreement if there are good reasons to not do this. 

I hope following story will convince you that default time limit should be small:

I worked on tests for new project and I got hanging tests. These tests were waiting for some event which not happens because of bugs. They will be failed after 1 minutes due to timeout.

But when you work on code you of course do not want to wait 1 minute. So it always forces manual interrupt of running tests. But then you could not run full test suite to see all problem tests. And also it is not really easy to detect current running test after interrupt. You need analyze stack in debugger and if you just close debugger information is lost.

So to fix it I change default timeout in each of my test cases.
If you will work on similar code you will need same approach to comfortably detect and fix "synchronization" bugs. 

So this was my story. I am sure default behaviour with small time limit will make SUnit much better.
Common case is that unit tests are supposed to be fast. Users expect it by default. And if something is going wrong fast tests should fail fast. 
When user wrote slow test it is expected to know that test is slow. And for such rare cases user can mark slow tests explicitly. But SUnit should not expect tests to be slow by default.

I think it is really good improvement for Pharo 6. It makes TDD in Pharo better.

Best regards,
Denis

Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

stepharong
In reply to this post by Guillermo Polito
On Thu, 16 Feb 2017 14:46:41 +0100, Guillermo Polito <[hidden email]> wrote:



On Thu, Feb 16, 2017 at 2:31 PM, Denis Kudriashov <[hidden email]> wrote:

2017-02-16 13:49 GMT+01:00 Guillermo Polito <[hidden email]>:
I vote for not introducing during code-freeze.

I understand it but these behaviour was introduced in Pharo 6. And It is already used quite long time. So it could be considered as small improvements to already integrated feature. 
 

We can discuss it for Pharo 7, I'm not against actually. But I have some concerns:

- How does it work when we are debugging?
I mean, imagine I'm running a test, a debugger is open, and I start working on it. How do these timeout currently work in this scenario? I imagine that the test runner should detect you entered into a "I'm debugging" mode and should not kill your test, but this

It solved very easily. "Watchdog process" just checks that test process isSuspended or not. If it is not suspended timeout is signalled. Test process is suspended only when debugged.

Ok. Is something like this in a class comment? If not, we could add it, because it will get lost in the mails.
+ 10000

 
 
is not clear to me how this works right now.

- Also, how can we better document these things? Because I was bitten a couple of times by this timeouts and the "I kill your process but I do not tell you", and it was not nice because it was completely silent or obscure.

Things which you show me I improved. Last days I fix Christophe issues which was really important. Anyway "fork management in tests" is not related to time limit of tests. And of course it is not supposed to make troubles but instead it should make life easy :)

I know, I'm not pleading for improvements :). Just talking about documenttion here. 




--
Using Opera's mail client: http://www.opera.com/mail/
Reply | Threaded
Open this post in threaded view
|

Re: 2 seconds default time limit for tests

Ben Coman
In reply to this post by Denis Kudriashov
When we come to do this, could there be some mechanism to scale *all*
these timeouts.
For example, to investigate VM crashes we might want to run all tests
under a debug VM
which would greatly extend the time taken for each test, and it would
be a pain to get a bundle of false positives.  Perhaps it might
autoscale by how many sends execute with one second in a loop before
tests are run, or similar.

cheers -ben

On Fri, Feb 17, 2017 at 6:03 PM, Denis Kudriashov <[hidden email]> wrote:

> I moved issue to Pharo 7
>
> 2017-02-16 13:33 GMT+01:00 Denis Kudriashov <[hidden email]>:
>>
>> Hi.
>>
>> In Pharo 6 we introduced time limit for tests. Currently it is set to 1
>> minute by default. And concrete test cases or single tests can redefine it
>> with suitable value. There is also setting to change default limit globally
>> which should help to not adopt external projects immediately if current
>> limit is bad for them.
>>
>> There is issue 19105 to make limit really small. It will set default limit
>> for 2 seconds. Slow tests are already marked with bigger value. So it is
>> safe for integration
>>
>> Here I ask for your agreement with this change. Or disagreement if there
>> are good reasons to not do this.
>>
>> I hope following story will convince you that default time limit should be
>> small:
>>
>> I worked on tests for new project and I got hanging tests. These tests
>> were waiting for some event which not happens because of bugs. They will be
>> failed after 1 minutes due to timeout.
>>
>> But when you work on code you of course do not want to wait 1 minute. So
>> it always forces manual interrupt of running tests. But then you could not
>> run full test suite to see all problem tests. And also it is not really easy
>> to detect current running test after interrupt. You need analyze stack in
>> debugger and if you just close debugger information is lost.
>>
>> So to fix it I change default timeout in each of my test cases.
>> If you will work on similar code you will need same approach to
>> comfortably detect and fix "synchronization" bugs.
>>
>> So this was my story. I am sure default behaviour with small time limit
>> will make SUnit much better.
>> Common case is that unit tests are supposed to be fast. Users expect it by
>> default. And if something is going wrong fast tests should fail fast.
>> When user wrote slow test it is expected to know that test is slow. And
>> for such rare cases user can mark slow tests explicitly. But SUnit should
>> not expect tests to be slow by default.
>>
>> I think it is really good improvement for Pharo 6. It makes TDD in Pharo
>> better.
>>
>> Best regards,
>> Denis
>
>