SUnit improvements need review and feedback
Hi. I am working on SUnit improvements. I open issue 19015 <https://pharo.fogbugz.com/f/cases/19015/Tests-should-never-hang-and-leave-fo...>. Slice is inbox which waits your review and feedback. I was trying to address three problems: *1) Tests should never hang. They should be always executed within time limit.* I give them 10 minutes for now to not change existing behaviour of tests. At next step it should be really reduced to ~100 milliseconds (?). Any TestCase could redefine time limit by method #defaultTimeLimit. Or it could be specified directly in test code by self timeLimit: 10 seconds (could be changed at any time of test execution). To implement this logic special watch dog process is running for given test suite to control execution time of tests. It is single process for full test suite. *2) When test completes all forked processes should be terminated.* If your tested code produced zombie processes SUnit will take care about destroying all such garbage. (it not means that you don't need to clean table in #tearDown but any code could be broken and running tests should not produce dirty system). *3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed. To implement this behaviour SUnit will handle errors from forked processes by suspending them and collecting them in special collection. I introduce TestFailedByForkedProcess error to signal these kind of problems at the end of tests. This error is resumable and resume will opens debuggers of suspended failures (in fact it will resume suspended processes). So to debug background failures you will need extra Proceed action on debugger when TestFailedByForkedProcess is signalled. But in normal run such tests will be just failed by TestFailedByForkedProcess error. *Now details on how it is done:* I introduce special process specific variable CurrentExecutionEnvironment. It is not installed by default and as default value it returns DefaultExecutionEnvironment instance. This variable is inheritable. If your install concrete environment into process it will be installed to any child process. So value of variable is instance of ExecutionEnvironment subclasses and you can install it by: anYourExecutionEnvironment beActiveDuring: aBlock When block completes previous environment is restored. For default environment there is class side method: DefaultExecutionEnvironment beActiveDuring: aBlock And to reset current environment to default: DefaultExecutionEnvironment beActive. SUnit introduces TestExecutionEnvironment which implements all described behaviour for time limits and forked processes. To activate environment there is new method #runCaseManaged. And submitted slice uses it instead of simple runCase. TestCase>>runCaseManaged CurrentExecutionEnvironment runTestCase: self DefaultExecutionEnvironment will install new TestExecutionEnvironment and delegate processing to it. And if TestExecutionEnvironment is already active it will just process new test case. Now monkey has problem in checking slice (annoying timeout for loading). So I can't see real system impact. But it should not stop you from review :)) I think it is very important features for all our tests. And environment idea will lead to very interesting future. Best regards, Denis
Denis can we also have assert: aNumber withPrecision: precision equals: otherNumber self assert: (aNumber round: precision) equals: otherNumber assert: aNumber closeTo: otherNumber assert: aNumber withPrecision: self defaultPrecision equals: otherNumber defaultPrecision ^ 2 Le 31/8/16 à 14:10, Denis Kudriashov a écrit :
Hi.
I am working on SUnit improvements. I open issue 19015 <https://pharo.fogbugz.com/f/cases/19015/Tests-should-never-hang-and-leave-fo...>. Slice is inbox which waits your review and feedback. I was trying to address three problems:
*1) Tests should never hang. They should be always executed within time limit.* I give them 10 minutes for now to not change existing behaviour of tests. At next step it should be really reduced to ~100 milliseconds (?). Any TestCase could redefine time limit by method #defaultTimeLimit. Or it could be specified directly in test code by self timeLimit: 10 seconds (could be changed at any time of test execution).
would be good
To implement this logic special watch dog process is running for given test suite to control execution time of tests. It is single process for full test suite.
*2) When test completes all forked processes should be terminated.***
If your tested code produced zombie processes SUnit will take care about destroying all such garbage. (it not means that you don't need to clean table in #tearDown but any code could be broken and running tests should not produce dirty system).
Yes!!!
*3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed.
Definitevely. Now when we execute tests from the testRunner and nautilus we should get the debugger, right?
To implement this behaviour SUnit will handle errors from forked processes by suspending them and collecting them in special collection. I introduce TestFailedByForkedProcess error to signal these kind of problems at the end of tests. This error is resumable and resume will opens debuggers of suspended failures (in fact it will resume suspended processes). So to debug background failures you will need extra Proceed action on debugger when TestFailedByForkedProcess is signalled. But in normal run such tests will be just failed by TestFailedByForkedProcess error. We should really write a little doc so that I can add it to the Sunit Chapter.
*Now details on how it is done:*
I introduce special process specific variable CurrentExecutionEnvironment. It is not installed by default and as default value it returns DefaultExecutionEnvironment instance. This variable is inheritable. If your install concrete environment into process it will be installed to any child process.
So value of variable is instance of ExecutionEnvironment subclasses and you can install it by:
anYourExecutionEnvironment beActiveDuring: aBlock
When block completes previous environment is restored. For default environment there is class side method:
DefaultExecutionEnvironment beActiveDuring: aBlock
And to reset current environment to default:
DefaultExecutionEnvironment beActive.
SUnit introduces TestExecutionEnvironment which implements all described behaviour for time limits and forked processes. To activate environment there is new method #runCaseManaged. And submitted slice uses it instead of simple runCase.
TestCase>>runCaseManaged CurrentExecutionEnvironment runTestCase: self
DefaultExecutionEnvironment will install new TestExecutionEnvironment and delegate processing to it. And if TestExecutionEnvironment is already active it will just process new test case.
Now monkey has problem in checking slice (annoying timeout for loading). So I can't see real system impact. But it should not stop you from review :)) I think it is very important features for all our tests. And environment idea will lead to very interesting future.
Indeed!
Best regards, Denis
For floating points it would be good to also have something related to unit of least precision like I think it exists in google tests: assert: aFloat isWithin: anInteger ulpFrom: anotherFloat ^(aFloat - anotherFloat) abs <= (anotherFloat ulp max: aFloat ulp) It's testing if the result are the same with a tolerance on least significant bits. (anotherFloat ulp max: aFloat ulp) is not really necessary, it's only for making the comparison symmetric. indeed, 2 and 2 predecessor do not have the same exponent, so do not have the same ulp. If we don't have symmetry, then self assert: 2.0 predecessor predecessor isWithin: 1 ulpFrom: 2.0. might differ from: self assert: 2.0 isWithin: 1 ulpFrom: 2.0 predecessor predecessor . Note that the this message cannot be used to test for zero result (because 0.0 ulp is the smallest denormal). For example: self assert: 2.0 - 2.0 predecessor isWithin: 4 ulpFrom: 0.0. would anwer false, but it makes no sense! One should write self assert: 2.0 isWithin: 4 ulpFrom: 2.0 predecessor. 2016-09-03 9:22 GMT+02:00 stepharo <stepharo@free.fr>:
Denis
can we also have
assert: aNumber withPrecision: precision equals: otherNumber
self assert: (aNumber round: precision) equals: otherNumber
assert: aNumber closeTo: otherNumber
assert: aNumber withPrecision: self defaultPrecision equals: otherNumber
defaultPrecision ^ 2
Le 31/8/16 à 14:10, Denis Kudriashov a écrit :
Hi.
I am working on SUnit improvements. I open issue 19015 <https://pharo.fogbugz.com/f/cases/19015/Tests-should-never-hang-and-leave-fo...>. Slice is inbox which waits your review and feedback. I was trying to address three problems:
*1) Tests should never hang. They should be always executed within time limit.*
I give them 10 minutes for now to not change existing behaviour of tests. At next step it should be really reduced to ~100 milliseconds (?). Any TestCase could redefine time limit by method #defaultTimeLimit. Or it could be specified directly in test code by self timeLimit: 10 seconds (could be changed at any time of test execution).
would be good
To implement this logic special watch dog process is running for given test suite to control execution time of tests. It is single process for full test suite.
*2) When test completes all forked processes should be terminated.*
If your tested code produced zombie processes SUnit will take care about destroying all such garbage. (it not means that you don't need to clean table in #tearDown but any code could be broken and running tests should not produce dirty system).
Yes!!!
*3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed.
Definitevely. Now when we execute tests from the testRunner and nautilus we should get the debugger, right?
To implement this behaviour SUnit will handle errors from forked processes by suspending them and collecting them in special collection. I introduce TestFailedByForkedProcess error to signal these kind of problems at the end of tests. This error is resumable and resume will opens debuggers of suspended failures (in fact it will resume suspended processes). So to debug background failures you will need extra Proceed action on debugger when TestFailedByForkedProcess is signalled. But in normal run such tests will be just failed by TestFailedByForkedProcess error.
We should really write a little doc so that I can add it to the Sunit Chapter.
*Now details on how it is done:*
I introduce special process specific variable CurrentExecutionEnvironment. It is not installed by default and as default value it returns DefaultExecutionEnvironment instance. This variable is inheritable. If your install concrete environment into process it will be installed to any child process.
So value of variable is instance of ExecutionEnvironment subclasses and you can install it by:
anYourExecutionEnvironment beActiveDuring: aBlock
When block completes previous environment is restored. For default environment there is class side method:
DefaultExecutionEnvironment beActiveDuring: aBlock
And to reset current environment to default:
DefaultExecutionEnvironment beActive.
SUnit introduces TestExecutionEnvironment which implements all described behaviour for time limits and forked processes. To activate environment there is new method #runCaseManaged. And submitted slice uses it instead of simple runCase.
TestCase>>runCaseManaged CurrentExecutionEnvironment runTestCase: self
DefaultExecutionEnvironment will install new TestExecutionEnvironment and delegate processing to it. And if TestExecutionEnvironment is already active it will just process new test case.
Now monkey has problem in checking slice (annoying timeout for loading). So I can't see real system impact. But it should not stop you from review :)) I think it is very important features for all our tests. And environment idea will lead to very interesting future.
Indeed!
Best regards, Denis
2016-09-03 9:48 GMT+02:00 Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com>:
For floating points it would be good to also have something related to unit of least precision like I think it exists in google tests:
assert: aFloat isWithin: anInteger ulpFrom: anotherFloat ^(aFloat - anotherFloat) abs <= (anotherFloat ulp max: aFloat ulp)
It's testing if the result are the same with a tolerance on least significant bits. (anotherFloat ulp max: aFloat ulp) is not really necessary, it's only for making the comparison symmetric. indeed, 2 and 2 predecessor do not have the same exponent, so do not have the same ulp. If we don't have symmetry, then self assert: 2.0 predecessor predecessor isWithin: 1 ulpFrom: 2.0. might differ from: self assert: 2.0 isWithin: 1 ulpFrom: 2.0 predecessor predecessor .
Note that the this message cannot be used to test for zero result (because 0.0 ulp is the smallest denormal). For example: self assert: 2.0 - 2.0 predecessor isWithin: 4 ulpFrom: 0.0. would anwer false, but it makes no sense! One should write self assert: 2.0 isWithin: 4 ulpFrom: 2.0 predecessor.
Bad result will be in other cases too (not only zero): expected := 8085- 8084.9999. computed := (80.85 * 100) - 8084.9999. (computed - expected) abs <= (computed ulp max: expected ulp) "==> false"
2016-09-03 11:17 GMT+02:00 Denis Kudriashov <dionisiydk@gmail.com>:
2016-09-03 9:48 GMT+02:00 Nicolas Cellier <nicolas.cellier.aka.nice@ gmail.com>:
For floating points it would be good to also have something related to unit of least precision like I think it exists in google tests:
assert: aFloat isWithin: anInteger ulpFrom: anotherFloat ^(aFloat - anotherFloat) abs <= (anotherFloat ulp max: aFloat ulp)
It's testing if the result are the same with a tolerance on least significant bits. (anotherFloat ulp max: aFloat ulp) is not really necessary, it's only for making the comparison symmetric. indeed, 2 and 2 predecessor do not have the same exponent, so do not have the same ulp. If we don't have symmetry, then self assert: 2.0 predecessor predecessor isWithin: 1 ulpFrom: 2.0. might differ from: self assert: 2.0 isWithin: 1 ulpFrom: 2.0 predecessor predecessor .
Note that the this message cannot be used to test for zero result (because 0.0 ulp is the smallest denormal). For example: self assert: 2.0 - 2.0 predecessor isWithin: 4 ulpFrom: 0.0. would anwer false, but it makes no sense! One should write self assert: 2.0 isWithin: 4 ulpFrom: 2.0 predecessor.
Bad result will be in other cases too (not only zero):
expected := 8085- 8084.9999. computed := (80.85 * 100) - 8084.9999. (computed - expected) abs <= (computed ulp max: expected ulp) "==> false"
Fine, this result is not within 1 ulp error because of catastrophic
cancellation, the difference is more than 1e7 ulp. That just mean that we have a numerical formulation that is quite unstable (it has half precision of double). I don't see why this invalidate this specific type of assertion, au contraire.
Nicolas I like your ideas can you propose (code) something? Stef Le 3/9/16 à 09:48, Nicolas Cellier a écrit :
For floating points it would be good to also have something related to unit of least precision like I think it exists in google tests:
assert: aFloat isWithin: anInteger ulpFrom: anotherFloat ^(aFloat - anotherFloat) abs <= (anotherFloat ulp max: aFloat ulp)
It's testing if the result are the same with a tolerance on least significant bits. (anotherFloat ulp max: aFloat ulp) is not really necessary, it's only for making the comparison symmetric. indeed, 2 and 2 predecessor do not have the same exponent, so do not have the same ulp. If we don't have symmetry, then self assert: 2.0 predecessor predecessor isWithin: 1 ulpFrom: 2.0. might differ from: self assert: 2.0 isWithin: 1 ulpFrom: 2.0 predecessor predecessor .
Note that the this message cannot be used to test for zero result (because 0.0 ulp is the smallest denormal). For example: self assert: 2.0 - 2.0 predecessor isWithin: 4 ulpFrom: 0.0. would anwer false, but it makes no sense! One should write self assert: 2.0 isWithin: 4 ulpFrom: 2.0 predecessor.
2016-09-03 9:22 GMT+02:00 stepharo <stepharo@free.fr <mailto:stepharo@free.fr>>:
Denis
can we also have
assert: aNumber withPrecision: precision equals: otherNumber
self assert: (aNumber round: precision) equals: otherNumber
assert: aNumber closeTo: otherNumber
assert: aNumber withPrecision: self defaultPrecision equals: otherNumber
defaultPrecision ^ 2
Le 31/8/16 à 14:10, Denis Kudriashov a écrit :
Hi.
I am working on SUnit improvements. I open issue 19015 <https://pharo.fogbugz.com/f/cases/19015/Tests-should-never-hang-and-leave-fo...>. Slice is inbox which waits your review and feedback. I was trying to address three problems:
*1) Tests should never hang. They should be always executed within time limit.* I give them 10 minutes for now to not change existing behaviour of tests. At next step it should be really reduced to ~100 milliseconds (?). Any TestCase could redefine time limit by method #defaultTimeLimit. Or it could be specified directly in test code by self timeLimit: 10 seconds (could be changed at any time of test execution).
would be good
To implement this logic special watch dog process is running for given test suite to control execution time of tests. It is single process for full test suite.
*2) When test completes all forked processes should be terminated.***
If your tested code produced zombie processes SUnit will take care about destroying all such garbage. (it not means that you don't need to clean table in #tearDown but any code could be broken and running tests should not produce dirty system).
Yes!!!
*3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed.
Definitevely. Now when we execute tests from the testRunner and nautilus we should get the debugger, right?
To implement this behaviour SUnit will handle errors from forked processes by suspending them and collecting them in special collection. I introduce TestFailedByForkedProcess error to signal these kind of problems at the end of tests. This error is resumable and resume will opens debuggers of suspended failures (in fact it will resume suspended processes). So to debug background failures you will need extra Proceed action on debugger when TestFailedByForkedProcess is signalled. But in normal run such tests will be just failed by TestFailedByForkedProcess error.
We should really write a little doc so that I can add it to the Sunit Chapter.
*Now details on how it is done:*
I introduce special process specific variable CurrentExecutionEnvironment. It is not installed by default and as default value it returns DefaultExecutionEnvironment instance. This variable is inheritable. If your install concrete environment into process it will be installed to any child process.
So value of variable is instance of ExecutionEnvironment subclasses and you can install it by:
anYourExecutionEnvironment beActiveDuring: aBlock
When block completes previous environment is restored. For default environment there is class side method:
DefaultExecutionEnvironment beActiveDuring: aBlock
And to reset current environment to default:
DefaultExecutionEnvironment beActive.
SUnit introduces TestExecutionEnvironment which implements all described behaviour for time limits and forked processes. To activate environment there is new method #runCaseManaged. And submitted slice uses it instead of simple runCase.
TestCase>>runCaseManaged CurrentExecutionEnvironment runTestCase: self
DefaultExecutionEnvironment will install new TestExecutionEnvironment and delegate processing to it. And if TestExecutionEnvironment is already active it will just process new test case.
Now monkey has problem in checking slice (annoying timeout for loading). So I can't see real system impact. But it should not stop you from review :)) I think it is very important features for all our tests. And environment idea will lead to very interesting future.
Indeed!
Best regards, Denis
Hi 2016-09-03 9:22 GMT+02:00 stepharo <stepharo@free.fr>:
*3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed.
Definitevely. Now when we execute tests from the testRunner and nautilus we should get the debugger, right?
Yes
2016-09-03 9:22 GMT+02:00 stepharo <stepharo@free.fr>:
Denis
can we also have
assert: aNumber withPrecision: precision equals: otherNumber
self assert: (aNumber round: precision) equals: otherNumber
assert: aNumber closeTo: otherNumber
assert: aNumber withPrecision: self defaultPrecision equals: otherNumber
defaultPrecision ^ 2
We could add it but as different issue. Also I really want move away from assertions to should expressions. It is much nicer to write things like this (which available in StateSpecs): 10.1234 should equal: 10.12 within: 0.01 "success" 10.1234 should equal: 10.12 within: 0.001 "failure" Also I has idea that float assertions should always use default precision (like 0.00000000001) without special message. (80.85 * 100) should equal: 8085 It will fail now but I want it always succeed in such cases. Because in most cases users don't care about this. And when it has real value users will specify concrete precision
2016-09-03 9:22 GMT+02:00 stepharo <stepharo@free.fr <mailto:stepharo@free.fr>>:
Denis
can we also have
assert: aNumber withPrecision: precision equals: otherNumber
self assert: (aNumber round: precision) equals: otherNumber
assert: aNumber closeTo: otherNumber
assert: aNumber withPrecision: self defaultPrecision equals: otherNumber
defaultPrecision ^ 2
We could add it but as different issue.
Also I really want move away from assertions to should expressions. It is much nicer to write things like this (which available in StateSpecs):
10.1234 should equal: 10.12 within: 0.01 "success" 10.1234 should equal: 10.12 within: 0.001 "failure"
Also I has idea that float assertions should always use default precision (like 0.00000000001) without special message.
(80.85 * 100) should equal: 8085
It will fail now but I want it always succeed in such cases. Because in most cases users don't care about this. And when it has real value users will specify concrete precision
Just one step at a time please. Let us make SUnit with some simple extension.
And it is worth breaking all the book chapters and videos. I think that assert: closeTo: kind of solution is - simple - regular with the other assert: - incremental So maximum quality. Stef
ARgh, since this got integrated I keep crashing images... I'm just executing some OSSubprocess like this: process := OSSUnixSubprocess new command: 'ls'; redirectStdout; runAndWaitOnExitDo: [ :command :outString | ^ outString ]. and my image crashes with a: (scheduler could not find a runnable process) So this is messing up badly the scheduler. We have to fix this :/ On Sun, Sep 4, 2016 at 9:58 AM, stepharo <stepharo@free.fr> wrote:
And it is worth breaking all the book chapters and videos. I think that assert: closeTo: kind of solution is - simple - regular with the other assert: - incremental
So maximum quality.
Stef
argh!!!!!! May be we should revert the change. And make the changes optional.
ARgh, since this got integrated I keep crashing images...
I'm just executing some OSSubprocess like this:
process := OSSUnixSubprocess new command: 'ls'; redirectStdout; runAndWaitOnExitDo: [ :command :outString | ^ outString ].
and my image crashes with a:
(scheduler could not find a runnable process)
So this is messing up badly the scheduler. We have to fix this :/
On Sun, Sep 4, 2016 at 9:58 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
And it is worth breaking all the book chapters and videos. I think that assert: closeTo: kind of solution is - simple - regular with the other assert: - incremental
So maximum quality.
Stef
I mean crash. Somehow my image's processes were killed and I only had the UI process: no delay process, no finalization, no idle process. Then, when the VM tries to suspend the current process and change to a second one, it finds none and it just quits. So it's not a VM problem I think but mostly something in the image. And in any case, now that I'm with a colder head (because I was mad after losing a couple of hours of work this afternoon :)) ): - first, I could not load OSSubprocess in latest pharo. I saw there were *two* test watcher processes and *killed* them. - then, I could load OSSubprocess, but it started crashing the image when running the tests from nautilus. - I tested with complex and simple OSSubprocess calls and I got always the same result - I even removed the child watcher process to see if that was the problem, but still crashing. In any case, I'm not sure that this is because of this addition, it's just a hunch for the moment. On Mon, Sep 5, 2016 at 7:47 PM, stepharo <stepharo@free.fr> wrote:
ARgh, since this got integrated I keep crashing images...
do you mean real crash or DNU. Because indeed I got some crashes too.
Guille, Just for the record, there has been some crashes in Pharo using OSSubprocess, but I (we) were never able to reproduce nor found the cause, and even less a fix :( So....any help in this direction is really appreciated as I want OSSubprocess to be robust. Cheers, On Mon, Sep 5, 2016 at 2:53 PM, Guillermo Polito <guillermopolito@gmail.com> wrote:
I mean crash.
Somehow my image's processes were killed and I only had the UI process: no delay process, no finalization, no idle process. Then, when the VM tries to suspend the current process and change to a second one, it finds none and it just quits.
So it's not a VM problem I think but mostly something in the image.
And in any case, now that I'm with a colder head (because I was mad after losing a couple of hours of work this afternoon :)) ):
- first, I could not load OSSubprocess in latest pharo. I saw there were *two* test watcher processes and *killed* them. - then, I could load OSSubprocess, but it started crashing the image when running the tests from nautilus. - I tested with complex and simple OSSubprocess calls and I got always the same result - I even removed the child watcher process to see if that was the problem, but still crashing.
In any case, I'm not sure that this is because of this addition, it's just a hunch for the moment.
On Mon, Sep 5, 2016 at 7:47 PM, stepharo <stepharo@free.fr> wrote:
ARgh, since this got integrated I keep crashing images...
do you mean real crash or DNU. Because indeed I got some crashes too.
-- Mariano http://marianopeck.wordpress.com
Hola, for the record, I'm using OSSubprocess in several projects and it's pretty robust. The problems I had were because either: - I was using it wrongly. e.g., I was deadlocking my process due to a process that was writing a lot to stdout. I fixed this after I read the entire documentation. I think that there should be a better warning at the beginning of the docs because all the starting examples do not use polling, and then this may create problems (because people read the documentation until they found what they needed, they will not read the entire thing ^^) - Or there was a library that was creating some noise with it. I remember at some point Seamless and OSSubprocess together were a crash factory. So I'm restarting now again with those two. The thing is that it is really frustrating to have crashes all day long with something that should not crash :( It's not that I'm messing with the VM, or that I do not know what I am doing... -------- Original Message --------
Guille,
Just for the record, there has been some crashes in Pharo using OSSubprocess, but I (we) were never able to reproduce nor found the cause, and even less a fix :( So....any help in this direction is really appreciated as I want OSSubprocess to be robust.
Cheers,
On Mon, Sep 5, 2016 at 2:53 PM, Guillermo Polito <guillermopolito@gmail.com <mailto:guillermopolito@gmail.com>> wrote:
I mean crash.
Somehow my image's processes were killed and I only had the UI process: no delay process, no finalization, no idle process. Then, when the VM tries to suspend the current process and change to a second one, it finds none and it just quits.
So it's not a VM problem I think but mostly something in the image.
And in any case, now that I'm with a colder head (because I was mad after losing a couple of hours of work this afternoon :)) ):
- first, I could not load OSSubprocess in latest pharo. I saw there were *two* test watcher processes and *killed* them. - then, I could load OSSubprocess, but it started crashing the image when running the tests from nautilus. - I tested with complex and simple OSSubprocess calls and I got always the same result - I even removed the child watcher process to see if that was the problem, but still crashing.
In any case, I'm not sure that this is because of this addition, it's just a hunch for the moment.
On Mon, Sep 5, 2016 at 7:47 PM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
ARgh, since this got integrated I keep crashing images...
do you mean real crash or DNU. Because indeed I got some crashes too.
-- Mariano http://marianopeck.wordpress.com
On Mon, Sep 5, 2016 at 3:18 PM, Guille Polito <guillermopolito@gmail.com> wrote:
Hola,
for the record, I'm using OSSubprocess in several projects and it's pretty robust.
Good to hear that.
The problems I had were because either:
- I was using it wrongly. e.g., I was deadlocking my process due to a process that was writing a lot to stdout. I fixed this after I read the entire documentation. I think that there should be a better warning at the beginning of the docs because all the starting examples do not use polling, and then this may create problems (because people read the documentation until they found what they needed, they will not read the entire thing ^^)
Indeed. But at the same time it's not that frequent to write that much into the stdout.
- Or there was a library that was creating some noise with it. I remember at some point Seamless and OSSubprocess together were a crash factory. So I'm restarting now again with those two.
The thing is that it is really frustrating to have crashes all day long with something that should not crash :( It's not that I'm messing with the VM, or that I do not know what I am doing...
Exactly. That's why I really want to fix that (even if this is not OSSubprocess fault). And I was unable so far :(
-------- Original Message --------
Guille,
Just for the record, there has been some crashes in Pharo using OSSubprocess, but I (we) were never able to reproduce nor found the cause, and even less a fix :( So....any help in this direction is really appreciated as I want OSSubprocess to be robust.
Cheers,
On Mon, Sep 5, 2016 at 2:53 PM, Guillermo Polito < guillermopolito@gmail.com> wrote:
I mean crash.
Somehow my image's processes were killed and I only had the UI process: no delay process, no finalization, no idle process. Then, when the VM tries to suspend the current process and change to a second one, it finds none and it just quits.
So it's not a VM problem I think but mostly something in the image.
And in any case, now that I'm with a colder head (because I was mad after losing a couple of hours of work this afternoon :)) ):
- first, I could not load OSSubprocess in latest pharo. I saw there were *two* test watcher processes and *killed* them. - then, I could load OSSubprocess, but it started crashing the image when running the tests from nautilus. - I tested with complex and simple OSSubprocess calls and I got always the same result - I even removed the child watcher process to see if that was the problem, but still crashing.
In any case, I'm not sure that this is because of this addition, it's just a hunch for the moment.
On Mon, Sep 5, 2016 at 7:47 PM, stepharo <stepharo@free.fr> wrote:
ARgh, since this got integrated I keep crashing images...
do you mean real crash or DNU. Because indeed I got some crashes too.
-- Mariano http://marianopeck.wordpress.com
-- Mariano http://marianopeck.wordpress.com
2016-09-05 18:23 GMT+02:00 Guillermo Polito <guillermopolito@gmail.com>:
ARgh, since this got integrated I keep crashing images...
I'm just executing some OSSubprocess like this:
process := OSSUnixSubprocess new command: 'ls'; redirectStdout; runAndWaitOnExitDo: [ :command :outString | ^ outString ].
and my image crashes with a:
(scheduler could not find a runnable process)
So this is messing up badly the scheduler. We have to fix this :/
To disable this changes just replace runCaseManaged with runCase in any senders. It will approve is it related or not. But if crash happens not during tests then it should not be related to this changes.
And fix for Guille case is inbox 19073 <https://pharo.fogbugz.com/f/cases/19073/Image-snapshot-should-be-forcible-ex...>. (details are inside) 2016-09-05 23:30 GMT+02:00 Denis Kudriashov <dionisiydk@gmail.com>:
2016-09-05 18:23 GMT+02:00 Guillermo Polito <guillermopolito@gmail.com>:
ARgh, since this got integrated I keep crashing images...
I'm just executing some OSSubprocess like this:
process := OSSUnixSubprocess new command: 'ls'; redirectStdout; runAndWaitOnExitDo: [ :command :outString | ^ outString ].
and my image crashes with a:
(scheduler could not find a runnable process)
So this is messing up badly the scheduler. We have to fix this :/
To disable this changes just replace runCaseManaged with runCase in any senders. It will approve is it related or not.
But if crash happens not during tests then it should not be related to this changes.
Tx denis. I wonder what is the reason of the crashes I experienced. Le 9/9/16 à 18:01, Denis Kudriashov a écrit :
And fix for Guille case is inbox 19073 <https://pharo.fogbugz.com/f/cases/19073/Image-snapshot-should-be-forcible-ex...>. (details are inside)
2016-09-05 23:30 GMT+02:00 Denis Kudriashov <dionisiydk@gmail.com <mailto:dionisiydk@gmail.com>>:
2016-09-05 18:23 GMT+02:00 Guillermo Polito <guillermopolito@gmail.com <mailto:guillermopolito@gmail.com>>:
ARgh, since this got integrated I keep crashing images...
I'm just executing some OSSubprocess like this:
process := OSSUnixSubprocess new command: 'ls'; redirectStdout; runAndWaitOnExitDo: [ :command :outString | ^ outString ].
and my image crashes with a:
(scheduler could not find a runnable process)
So this is messing up badly the scheduler. We have to fix this :/
To disable this changes just replace runCaseManaged with runCase in any senders. It will approve is it related or not.
But if crash happens not during tests then it should not be related to this changes.
2016-09-09 18:09 GMT+02:00 stepharo <stepharo@free.fr>:
Tx denis.
I wonder what is the reason of the crashes I experienced.
I don't know. Here is details on covered issue: TestExecutionEnvironment introduces problem when image is saved inside test process (which is case for Guille code which fork image inside test). And problem is that snapshot restarts all low level processes like IdlProcess, eventLoop and others (which are spawned in startup list). And when it happens inside test process all child processes are intercepted and terminated when test completes which means killing all system processes on test completes To fix it image snapshot should be always performed under default execution environment. (There is already another case like this: opening debugger to debug test. Debugger spawns new UI process and it should not be under test environment).
Super!!! It's integrated (In 60201). Thank's integrators. Now we need correct default time limit and mark long tests as long. I opened new issue 19035 <https://pharo.fogbugz.com/f/cases/19035/Default-time-limit-for-tests-should-...> . What you think about default value? I would use 200 milliseconds. 2016-08-31 14:10 GMT+02:00 Denis Kudriashov <dionisiydk@gmail.com>:
Hi.
I am working on SUnit improvements. I open issue 19015 <https://pharo.fogbugz.com/f/cases/19015/Tests-should-never-hang-and-leave-fo...>. Slice is inbox which waits your review and feedback. I was trying to address three problems:
*1) Tests should never hang. They should be always executed within time limit.*
I give them 10 minutes for now to not change existing behaviour of tests. At next step it should be really reduced to ~100 milliseconds (?). Any TestCase could redefine time limit by method #defaultTimeLimit. Or it could be specified directly in test code by self timeLimit: 10 seconds (could be changed at any time of test execution).
To implement this logic special watch dog process is running for given test suite to control execution time of tests. It is single process for full test suite.
*2) When test completes all forked processes should be terminated.*
If your tested code produced zombie processes SUnit will take care about destroying all such garbage. (it not means that you don't need to clean table in #tearDown but any code could be broken and running tests should not produce dirty system).
*3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed.
To implement this behaviour SUnit will handle errors from forked processes by suspending them and collecting them in special collection. I introduce TestFailedByForkedProcess error to signal these kind of problems at the end of tests. This error is resumable and resume will opens debuggers of suspended failures (in fact it will resume suspended processes). So to debug background failures you will need extra Proceed action on debugger when TestFailedByForkedProcess is signalled. But in normal run such tests will be just failed by TestFailedByForkedProcess error.
*Now details on how it is done:*
I introduce special process specific variable CurrentExecutionEnvironment. It is not installed by default and as default value it returns DefaultExecutionEnvironment instance. This variable is inheritable. If your install concrete environment into process it will be installed to any child process.
So value of variable is instance of ExecutionEnvironment subclasses and you can install it by:
anYourExecutionEnvironment beActiveDuring: aBlock
When block completes previous environment is restored. For default environment there is class side method:
DefaultExecutionEnvironment beActiveDuring: aBlock
And to reset current environment to default:
DefaultExecutionEnvironment beActive.
SUnit introduces TestExecutionEnvironment which implements all described behaviour for time limits and forked processes. To activate environment there is new method #runCaseManaged. And submitted slice uses it instead of simple runCase.
TestCase>>runCaseManaged CurrentExecutionEnvironment runTestCase: self
DefaultExecutionEnvironment will install new TestExecutionEnvironment and delegate processing to it. And if TestExecutionEnvironment is already active it will just process new test case.
Now monkey has problem in checking slice (annoying timeout for loading). So I can't see real system impact. But it should not stop you from review :)) I think it is very important features for all our tests. And environment idea will lead to very interesting future.
Best regards, Denis
How do you propose raising the limit for a specific test would go? Pragma? Denis Kudriashov wrote
Super!!! It's integrated (In 60201). Thank's integrators.
Now we need correct default time limit and mark long tests as long. I opened new issue 19035 <https://pharo.fogbugz.com/f/cases/19035/Default-time-limit-for-tests-should-...; . What you think about default value? I would use 200 milliseconds.
2016-08-31 14:10 GMT+02:00 Denis Kudriashov <
dionisiydk@
>:
Hi.
I am working on SUnit improvements. I open issue 19015 <https://pharo.fogbugz.com/f/cases/19015/Tests-should-never-hang-and-leave-fo...;. Slice is inbox which waits your review and feedback. I was trying to address three problems:
*1) Tests should never hang. They should be always executed within time limit.*
I give them 10 minutes for now to not change existing behaviour of tests. At next step it should be really reduced to ~100 milliseconds (?). Any TestCase could redefine time limit by method #defaultTimeLimit. Or it could be specified directly in test code by self timeLimit: 10 seconds (could be changed at any time of test execution).
To implement this logic special watch dog process is running for given test suite to control execution time of tests. It is single process for full test suite.
*2) When test completes all forked processes should be terminated.*
If your tested code produced zombie processes SUnit will take care about destroying all such garbage. (it not means that you don't need to clean table in #tearDown but any code could be broken and running tests should not produce dirty system).
*3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed.
To implement this behaviour SUnit will handle errors from forked processes by suspending them and collecting them in special collection. I introduce TestFailedByForkedProcess error to signal these kind of problems at the end of tests. This error is resumable and resume will opens debuggers of suspended failures (in fact it will resume suspended processes). So to debug background failures you will need extra Proceed action on debugger when TestFailedByForkedProcess is signalled. But in normal run such tests will be just failed by TestFailedByForkedProcess error.
*Now details on how it is done:*
I introduce special process specific variable CurrentExecutionEnvironment. It is not installed by default and as default value it returns DefaultExecutionEnvironment instance. This variable is inheritable. If your install concrete environment into process it will be installed to any child process.
So value of variable is instance of ExecutionEnvironment subclasses and you can install it by:
anYourExecutionEnvironment beActiveDuring: aBlock
When block completes previous environment is restored. For default environment there is class side method:
DefaultExecutionEnvironment beActiveDuring: aBlock
And to reset current environment to default:
DefaultExecutionEnvironment beActive.
SUnit introduces TestExecutionEnvironment which implements all described behaviour for time limits and forked processes. To activate environment there is new method #runCaseManaged. And submitted slice uses it instead of simple runCase.
TestCase>>runCaseManaged CurrentExecutionEnvironment runTestCase: self
DefaultExecutionEnvironment will install new TestExecutionEnvironment and delegate processing to it. And if TestExecutionEnvironment is already active it will just process new test case.
Now monkey has problem in checking slice (annoying timeout for loading). So I can't see real system impact. But it should not stop you from review :)) I think it is very important features for all our tests. And environment idea will lead to very interesting future.
Best regards, Denis
-- View this message in context: http://forum.world.st/SUnit-improvements-need-review-and-feedback-tp4913413p... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
Oh wait I see in the first message in this thread you have self timeLimit: 10 seconds Paul DeBruicker wrote
How do you propose raising the limit for a specific test would go?
Pragma? Denis Kudriashov wrote
Super!!! It's integrated (In 60201). Thank's integrators.
Now we need correct default time limit and mark long tests as long. I opened new issue 19035 <https://pharo.fogbugz.com/f/cases/19035/Default-time-limit-for-tests-should-...; . What you think about default value? I would use 200 milliseconds.
2016-08-31 14:10 GMT+02:00 Denis Kudriashov <
dionisiydk@
>:
Hi.
I am working on SUnit improvements. I open issue 19015 <https://pharo.fogbugz.com/f/cases/19015/Tests-should-never-hang-and-leave-fo...;. Slice is inbox which waits your review and feedback. I was trying to address three problems:
*1) Tests should never hang. They should be always executed within time limit.*
I give them 10 minutes for now to not change existing behaviour of tests. At next step it should be really reduced to ~100 milliseconds (?). Any TestCase could redefine time limit by method #defaultTimeLimit. Or it could be specified directly in test code by self timeLimit: 10 seconds (could be changed at any time of test execution).
To implement this logic special watch dog process is running for given test suite to control execution time of tests. It is single process for full test suite.
*2) When test completes all forked processes should be terminated.*
If your tested code produced zombie processes SUnit will take care about destroying all such garbage. (it not means that you don't need to clean table in #tearDown but any code could be broken and running tests should not produce dirty system).
*3) Any failures inside forked processes should not spawn debugger while running tests.* Only when we debug tests we need debugger on forked failed processes. During normal run SUnit should prevent such "background debuggers" and mark such tests as failed.
To implement this behaviour SUnit will handle errors from forked processes by suspending them and collecting them in special collection. I introduce TestFailedByForkedProcess error to signal these kind of problems at the end of tests. This error is resumable and resume will opens debuggers of suspended failures (in fact it will resume suspended processes). So to debug background failures you will need extra Proceed action on debugger when TestFailedByForkedProcess is signalled. But in normal run such tests will be just failed by TestFailedByForkedProcess error.
*Now details on how it is done:*
I introduce special process specific variable CurrentExecutionEnvironment. It is not installed by default and as default value it returns DefaultExecutionEnvironment instance. This variable is inheritable. If your install concrete environment into process it will be installed to any child process.
So value of variable is instance of ExecutionEnvironment subclasses and you can install it by:
anYourExecutionEnvironment beActiveDuring: aBlock
When block completes previous environment is restored. For default environment there is class side method:
DefaultExecutionEnvironment beActiveDuring: aBlock
And to reset current environment to default:
DefaultExecutionEnvironment beActive.
SUnit introduces TestExecutionEnvironment which implements all described behaviour for time limits and forked processes. To activate environment there is new method #runCaseManaged. And submitted slice uses it instead of simple runCase.
TestCase>>runCaseManaged CurrentExecutionEnvironment runTestCase: self
DefaultExecutionEnvironment will install new TestExecutionEnvironment and delegate processing to it. And if TestExecutionEnvironment is already active it will just process new test case.
Now monkey has problem in checking slice (annoying timeout for loading). So I can't see real system impact. But it should not stop you from review :)) I think it is very important features for all our tests. And environment idea will lead to very interesting future.
Best regards, Denis
-- View this message in context: http://forum.world.st/SUnit-improvements-need-review-and-feedback-tp4913413p... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
participants (7)
-
Denis Kudriashov -
Guille Polito -
Guillermo Polito
-
Mariano Martinez Peck -
Nicolas Cellier -
Paul DeBruicker -
stepharo