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 <https://pharo.fogbugz.com/f/cases/19105/Default-time-limit-for-tests-should-...> 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
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 <dionisiydk@gmail.com> 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 <https://pharo.fogbugz.com/f/cases/19105/Default-time-limit-for-tests-should-...> 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
On 16 Feb 2017, at 13:49, Guillermo Polito <guillermopolito@gmail.com> 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 <dionisiydk@gmail.com <mailto:dionisiydk@gmail.com>> 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 <https://pharo.fogbugz.com/f/cases/19105/Default-time-limit-for-tests-should-...> 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
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 <estebanlm@gmail.com> wrote:
On 16 Feb 2017, at 13:49, Guillermo Polito <guillermopolito@gmail.com> 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 <dionisiydk@gmail.com> 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
2017-02-16 14:26 GMT+01:00 Sven Van Caekenberghe <sven@stfx.eu>:
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.
On 16 Feb 2017, at 14:32, Denis Kudriashov <dionisiydk@gmail.com> wrote:
2017-02-16 14:26 GMT+01:00 Sven Van Caekenberghe <sven@stfx.eu>: 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 ?
2017-02-16 14:35 GMT+01:00 Sven Van Caekenberghe <sven@stfx.eu>:
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.
On Thu, Feb 16, 2017 at 9:32 PM, Denis Kudriashov <dionisiydk@gmail.com> wrote:
2017-02-16 14:26 GMT+01:00 Sven Van Caekenberghe <sven@stfx.eu>:
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
2017-02-16 14:59 GMT+01:00 Ben Coman <btc@openinworld.com>:
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.
2017-02-16 13:49 GMT+01:00 Guillermo Polito <guillermopolito@gmail.com>:
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 :)
On Thu, Feb 16, 2017 at 2:31 PM, Denis Kudriashov <dionisiydk@gmail.com> wrote:
2017-02-16 13:49 GMT+01:00 Guillermo Polito <guillermopolito@gmail.com>:
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.
2017-02-16 14:46 GMT+01:00 Guillermo Polito <guillermopolito@gmail.com>:
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.
On Thu, 16 Feb 2017 14:46:41 +0100, Guillermo Polito <guillermopolito@gmail.com> wrote:
On Thu, Feb 16, 2017 at 2:31 PM, Denis Kudriashov <dionisiydk@gmail.com> wrote:
2017-02-16 13:49 GMT+01:00 Guillermo Polito <guillermopolito@gmail.com>:
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/
I moved issue to Pharo 7 2017-02-16 13:33 GMT+01:00 Denis Kudriashov <dionisiydk@gmail.com>:
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 <https://pharo.fogbugz.com/f/cases/19105/Default-time-limit-for-tests-should-...> 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
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 <dionisiydk@gmail.com> wrote:
I moved issue to Pharo 7
2017-02-16 13:33 GMT+01:00 Denis Kudriashov <dionisiydk@gmail.com>:
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
participants (6)
-
Ben Coman -
Denis Kudriashov -
Esteban Lorenzano -
Guillermo Polito -
stepharong -
Sven Van Caekenberghe