2 seconds default time limit for tests
Locked
2 seconds default time limit for tests
 
|
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 |
Re: 2 seconds default time limit for tests
|
|
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:
|
|
|
+1: not in code freeze.
|
Re: 2 seconds default time limit for tests
|
|
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 >> > |
Re: 2 seconds default time limit for tests
|
|
In reply to this post by Guillermo Polito
2017-02-16 13:49 GMT+01:00 Guillermo Polito <[hidden email]>:
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.
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.
|
Re: 2 seconds default time limit for tests
|
|
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. |
Re: 2 seconds default time limit for tests
|
|
> 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 ? |
Re: 2 seconds default time limit for tests
|
|
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. |
Re: 2 seconds default time limit for tests
|
|
In reply to this post by Denis Kudriashov
On Thu, Feb 16, 2017 at 2:31 PM, Denis Kudriashov <[hidden email]> wrote:
Ok. Is something like this in a class comment? If not, we could add it, because it will get lost in the mails.
I know, I'm not pleading for improvements :). Just talking about documenttion here. |
|
|
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 |
Re: 2 seconds default time limit for tests
|
|
In reply to this post by Guillermo Polito
2017-02-16 14:46 GMT+01:00 Guillermo Polito <[hidden email]>:
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. |
Re: 2 seconds default time limit for tests
|
|
In reply to this post by Ben Coman
2017-02-16 14:59 GMT+01:00 Ben Coman <[hidden email]>:
I would not wrote this mail without Monkey :). With "2 seconds" Monkey was happy.
|
Re: 2 seconds default time limit for tests
|
|
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]>:
|
Re: 2 seconds default time limit for tests
|
|
In reply to this post by Guillermo Polito
On Thu, 16 Feb 2017 14:46:41 +0100, Guillermo Polito <[hidden email]> wrote:
+ 10000
-- Using Opera's mail client: http://www.opera.com/mail/ |
|
|
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 > > |
«
Return to Pharo Smalltalk Developers
|
1 view|%1 views
| Free forum by Nabble | Edit this page |