Christoph Thiede uploaded a new version of SUnit to project The Inbox: http://source.squeak.org/inbox/SUnit-ct.129.mcz
==================== Summary ====================
Name: SUnit-ct.129 Author: ct Time: 24 September 2020, 10:42:52.868426 am UUID: 92e68d23-8472-5d48-96d3-8435bd56ac14 Ancestors: SUnit-pre.122
Proposal: Catch warnings and halts in test case execution as well as Errors.
Catching (Error, Warning, Halt) is a common pattern to be (relatively) sure that no debugger will occur during an operation. For related usages, see Morph >> #fullBounds, WorldState >> #displayWorldSafely:, and many other places. IMO it is no desired behavior that the whole test execution, i.e. in a TestRunner, is interrupted because any method under test contains a halt or raises a DeprecationWarning, for example. Instead, the test should be listed as red.
For a similar discussion, see https://github.com/hpi-swa/smalltalkCI/issues/470. I believe we already had talked about this on squeak-dev, but if I remember correctly, I cannot find the thread again.
=============== Diff against SUnit-pre.122 ===============
Item was changed: ----- Method: TestCase>>timeout:after: (in category 'private') ----- timeout: aBlock after: seconds "Evaluate the argument block. Time out if the evaluation is not complete after the given number of seconds. Handle the situation that a timeout may occur after a failure (during debug)"
| theProcess delay watchdog |
"the block will be executed in the current process" theProcess := Processor activeProcess. delay := Delay forSeconds: seconds.
"make a watchdog process" watchdog := [ delay wait. "wait for timeout or completion" theProcess ifNotNil:[ theProcess signalException: (TestFailure new messageText: 'Test timed out') ] ] newProcess.
"Watchdog needs to run at high priority to do its job (but not at timing priority)" watchdog priority: Processor timingPriority-1.
"catch the timeout signal" watchdog resume. "start up the watchdog" + ^[aBlock on: TestFailure, (Error, Warning, Halt) do: [:ex| - ^[aBlock on: TestFailure, Error, Halt do:[:ex| theProcess := nil. ex pass. ]] ensure:[ "evaluate the receiver" theProcess := nil. "it has completed, so ..." delay delaySemaphore signal. "arrange for the watchdog to exit" ]!
Item was added: + ----- Method: TestResult class>>exAllErrors (in category 'exceptions') ----- + exAllErrors + ^ self exError, Warning, Halt + !
Item was changed: ----- Method: TestResult>>runCase: (in category 'running') ----- runCase: aTestCase | testCasePassed timeToRun | testCasePassed := true.
[timeToRun := [aTestCase runCase] timeToRunWithoutGC] on: self class failure do: [:signal | failures add: aTestCase. testCasePassed := false. signal return: false] + on: self class exAllErrors - on: self class error do: [:signal | errors add: aTestCase. testCasePassed := false. signal return: false]. testCasePassed ifTrue: [passed add: aTestCase]. self durations at: aTestCase put: timeToRun.!
Hi Christoph,
If you run all tests in your image without and with your change, does the number of test failures change?
Fabio
On Thu, Sep 24, 2020 at 10:42 AM commits@source.squeak.org wrote:
Christoph Thiede uploaded a new version of SUnit to project The Inbox: http://source.squeak.org/inbox/SUnit-ct.129.mcz
==================== Summary ====================
Name: SUnit-ct.129 Author: ct Time: 24 September 2020, 10:42:52.868426 am UUID: 92e68d23-8472-5d48-96d3-8435bd56ac14 Ancestors: SUnit-pre.122
Proposal: Catch warnings and halts in test case execution as well as Errors.
Catching (Error, Warning, Halt) is a common pattern to be (relatively) sure that no debugger will occur during an operation. For related usages, see Morph >> #fullBounds, WorldState >> #displayWorldSafely:, and many other places. IMO it is no desired behavior that the whole test execution, i.e. in a TestRunner, is interrupted because any method under test contains a halt or raises a DeprecationWarning, for example. Instead, the test should be listed as red.
For a similar discussion, see https://github.com/hpi-swa/smalltalkCI/issues/470. I believe we already had talked about this on squeak-dev, but if I remember correctly, I cannot find the thread again.
=============== Diff against SUnit-pre.122 ===============
Item was changed: ----- Method: TestCase>>timeout:after: (in category 'private') ----- timeout: aBlock after: seconds "Evaluate the argument block. Time out if the evaluation is not complete after the given number of seconds. Handle the situation that a timeout may occur after a failure (during debug)"
| theProcess delay watchdog | "the block will be executed in the current process" theProcess := Processor activeProcess. delay := Delay forSeconds: seconds. "make a watchdog process" watchdog := [ delay wait. "wait for timeout or completion" theProcess ifNotNil:[ theProcess signalException: (TestFailure new messageText: 'Test timed out') ] ] newProcess. "Watchdog needs to run at high priority to do its job (but not at timing priority)" watchdog priority: Processor timingPriority-1. "catch the timeout signal" watchdog resume. "start up the watchdog"
^[aBlock on: TestFailure, (Error, Warning, Halt) do: [:ex|
^[aBlock on: TestFailure, Error, Halt do:[:ex| theProcess := nil. ex pass. ]] ensure:[ "evaluate the receiver" theProcess := nil. "it has completed, so ..." delay delaySemaphore signal. "arrange for the watchdog to exit" ]!
Item was added:
- ----- Method: TestResult class>>exAllErrors (in category 'exceptions') -----
- exAllErrors
^ self exError, Warning, Halt
!
Item was changed: ----- Method: TestResult>>runCase: (in category 'running') ----- runCase: aTestCase
| testCasePassed timeToRun | testCasePassed := true. [timeToRun := [aTestCase runCase] timeToRunWithoutGC] on: self class failure do: [:signal | failures add: aTestCase. testCasePassed := false. signal return: false]
on: self class exAllErrors
on: self class error do: [:signal | errors add: aTestCase. testCasePassed := false. signal return: false]. testCasePassed ifTrue: [passed add: aTestCase]. self durations at: aTestCase put: timeToRun.!
Hi Fabio, hi all,
very good question (and after having waited about twenty minutes for two test executions and comparing them manually, I once again wish we had CI for every inbox commits ...)!
And actually, I broke the DecompilerTests, e.g. #testBlockNumbering, because somewhere during compilation, an UndeclaredVariableWarning is raised. All other tests, however, did not change their result after loading this inbox commit.
This behavior leads me to the question of whether it is justified to derive UndeclaredVariableWarning from Warning, given its #defaultAction implementation making it behave like a Notification.
Alternatively, what could we do else if it is not acceptable to interrupt a test once it raises a warning? As I proposed somewhere else, we could catch UnhandledError instead of all these classes eventually open a debugger, but Marcel told me that this would be an implementation detail of the EHS and should not be exposed. Still, I cannot imagine any other way to write an exception handler that detects whether an exception will "raise a problem" eventually or whether it won't. Hmm ... What do you think? :-)
Best,
Christoph
http://www.hpi.de/ ________________________________ Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Fabio Niephaus lists@fniephaus.com Gesendet: Donnerstag, 24. September 2020 10:50:44 An: squeak-dev@lists.squeakfoundation.org Betreff: Re: [squeak-dev] The Inbox: SUnit-ct.129.mcz
Hi Christoph,
If you run all tests in your image without and with your change, does the number of test failures change?
Fabio
On Thu, Sep 24, 2020 at 10:42 AM commits@source.squeak.org wrote:
Christoph Thiede uploaded a new version of SUnit to project The Inbox: http://source.squeak.org/inbox/SUnit-ct.129.mcz
==================== Summary ====================
Name: SUnit-ct.129 Author: ct Time: 24 September 2020, 10:42:52.868426 am UUID: 92e68d23-8472-5d48-96d3-8435bd56ac14 Ancestors: SUnit-pre.122
Proposal: Catch warnings and halts in test case execution as well as Errors.
Catching (Error, Warning, Halt) is a common pattern to be (relatively) sure that no debugger will occur during an operation. For related usages, see Morph >> #fullBounds, WorldState >> #displayWorldSafely:, and many other places. IMO it is no desired behavior that the whole test execution, i.e. in a TestRunner, is interrupted because any method under test contains a halt or raises a DeprecationWarning, for example. Instead, the test should be listed as red.
For a similar discussion, see https://github.com/hpi-swa/smalltalkCI/issues/470. I believe we already had talked about this on squeak-dev, but if I remember correctly, I cannot find the thread again.
=============== Diff against SUnit-pre.122 ===============
Item was changed: ----- Method: TestCase>>timeout:after: (in category 'private') ----- timeout: aBlock after: seconds "Evaluate the argument block. Time out if the evaluation is not complete after the given number of seconds. Handle the situation that a timeout may occur after a failure (during debug)"
| theProcess delay watchdog | "the block will be executed in the current process" theProcess := Processor activeProcess. delay := Delay forSeconds: seconds. "make a watchdog process" watchdog := [ delay wait. "wait for timeout or completion" theProcess ifNotNil:[ theProcess signalException: (TestFailure new messageText: 'Test timed out') ] ] newProcess. "Watchdog needs to run at high priority to do its job (but not at timing priority)" watchdog priority: Processor timingPriority-1. "catch the timeout signal" watchdog resume. "start up the watchdog"
^[aBlock on: TestFailure, (Error, Warning, Halt) do: [:ex|
^[aBlock on: TestFailure, Error, Halt do:[:ex| theProcess := nil. ex pass. ]] ensure:[ "evaluate the receiver" theProcess := nil. "it has completed, so ..." delay delaySemaphore signal. "arrange for the watchdog to exit" ]!
Item was added:
- ----- Method: TestResult class>>exAllErrors (in category 'exceptions') -----
- exAllErrors
^ self exError, Warning, Halt
!
Item was changed: ----- Method: TestResult>>runCase: (in category 'running') ----- runCase: aTestCase
| testCasePassed timeToRun | testCasePassed := true. [timeToRun := [aTestCase runCase] timeToRunWithoutGC] on: self class failure do: [:signal | failures add: aTestCase. testCasePassed := false. signal return: false]
on: self class exAllErrors
on: self class error do: [:signal | errors add: aTestCase. testCasePassed := false. signal return: false]. testCasePassed ifTrue: [passed add: aTestCase]. self durations at: aTestCase put: timeToRun.!
Hi all,
I would like to raise this topic again.
tl;dr: I proposed to treat all warnings raised in a test case as an error of the test, similar as we already do for Halts. Currently, a test that raises a warning will break the entire test runner. Do you agree with that idea in general? :-)
If yes, I can also update the DecompilerTests so that they do not raise a UndeclaredVariableWarning any longer.
Best, Christoph
--- Sent from Squeak Inbox Talk
On 2020-09-24T10:22:19+00:00, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Fabio, hi all,
very good question (and after having waited about twenty minutes for two test executions and comparing them manually, I once again wish we had CI for every inbox commits ...)!
And actually, I broke the DecompilerTests, e.g. #testBlockNumbering, because somewhere during compilation, an UndeclaredVariableWarning is raised. All other tests, however, did not change their result after loading this inbox commit.
This behavior leads me to the question of whether it is justified to derive UndeclaredVariableWarning from Warning, given its #defaultAction implementation making it behave like a Notification.
Alternatively, what could we do else if it is not acceptable to interrupt a test once it raises a warning? As I proposed somewhere else, we could catch UnhandledError instead of all these classes eventually open a debugger, but Marcel told me that this would be an implementation detail of the EHS and should not be exposed. Still, I cannot imagine any other way to write an exception handler that detects whether an exception will "raise a problem" eventually or whether it won't. Hmm ... What do you think? :-)
Best,
Christoph
http://www.hpi.de/ ________________________________ Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von Fabio Niephaus <lists at fniephaus.com> Gesendet: Donnerstag, 24. September 2020 10:50:44 An: squeak-dev at lists.squeakfoundation.org Betreff: Re: [squeak-dev] The Inbox: SUnit-ct.129.mcz
Hi Christoph,
If you run all tests in your image without and with your change, does the number of test failures change?
Fabio
On Thu, Sep 24, 2020 at 10:42 AM <commits at source.squeak.org> wrote:
Christoph Thiede uploaded a new version of SUnit to project The Inbox: http://source.squeak.org/inbox/SUnit-ct.129.mcz
==================== Summary ====================
Name: SUnit-ct.129 Author: ct Time: 24 September 2020, 10:42:52.868426 am UUID: 92e68d23-8472-5d48-96d3-8435bd56ac14 Ancestors: SUnit-pre.122
Proposal: Catch warnings and halts in test case execution as well as Errors.
Catching (Error, Warning, Halt) is a common pattern to be (relatively) sure that no debugger will occur during an operation. For related usages, see Morph >> #fullBounds, WorldState >> #displayWorldSafely:, and many other places. IMO it is no desired behavior that the whole test execution, i.e. in a TestRunner, is interrupted because any method under test contains a halt or raises a DeprecationWarning, for example. Instead, the test should be listed as red.
For a similar discussion, see https://github.com/hpi-swa/smalltalkCI/issues/470. I believe we already had talked about this on squeak-dev, but if I remember correctly, I cannot find the thread again.
=============== Diff against SUnit-pre.122 ===============
Item was changed: ----- Method: TestCase>>timeout:after: (in category 'private') ----- timeout: aBlock after: seconds "Evaluate the argument block. Time out if the evaluation is not complete after the given number of seconds. Handle the situation that a timeout may occur after a failure (during debug)"
| theProcess delay watchdog | "the block will be executed in the current process" theProcess := Processor activeProcess. delay := Delay forSeconds: seconds. "make a watchdog process" watchdog := [ delay wait. "wait for timeout or completion" theProcess ifNotNil:[ theProcess signalException: (TestFailure new messageText: 'Test timed out') ] ] newProcess. "Watchdog needs to run at high priority to do its job (but not at timing priority)" watchdog priority: Processor timingPriority-1. "catch the timeout signal" watchdog resume. "start up the watchdog"
^[aBlock on: TestFailure, (Error, Warning, Halt) do: [:ex|
^[aBlock on: TestFailure, Error, Halt do:[:ex| theProcess := nil. ex pass. ]] ensure:[ "evaluate the receiver" theProcess := nil. "it has completed, so ..." delay delaySemaphore signal. "arrange for the watchdog to exit" ]!
Item was added:
- ----- Method: TestResult class>>exAllErrors (in category 'exceptions') -----
- exAllErrors
^ self exError, Warning, Halt
!
Item was changed: ----- Method: TestResult>>runCase: (in category 'running') ----- runCase: aTestCase
| testCasePassed timeToRun | testCasePassed := true. [timeToRun := [aTestCase runCase] timeToRunWithoutGC] on: self class failure do: [:signal | failures add: aTestCase. testCasePassed := false. signal return: false]
on: self class exAllErrors
on: self class error do: [:signal | errors add: aTestCase. testCasePassed := false. signal return: false]. testCasePassed ifTrue: [passed add: aTestCase]. self durations at: aTestCase put: timeToRun.!
Hi Christoph --
Do you agree with that idea in general? :-)
+1 Do it. Make all cases of "Warning" or "Halt" end up as a red test result.
Best, Marcel Am 10.01.2022 18:22:11 schrieb christoph.thiede@student.hpi.uni-potsdam.de christoph.thiede@student.hpi.uni-potsdam.de: Hi all,
I would like to raise this topic again.
tl;dr: I proposed to treat all warnings raised in a test case as an error of the test, similar as we already do for Halts. Currently, a test that raises a warning will break the entire test runner. Do you agree with that idea in general? :-)
If yes, I can also update the DecompilerTests so that they do not raise a UndeclaredVariableWarning any longer.
Best, Christoph
--- Sent from Squeak Inbox Talk [https://github.com/hpi-swa-lab/squeak-inbox-talk]
On 2020-09-24T10:22:19+00:00, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Fabio, hi all,
very good question (and after having waited about twenty minutes for two test executions and comparing them manually, I once again wish we had CI for every inbox commits ...)!
And actually, I broke the DecompilerTests, e.g. #testBlockNumbering, because somewhere during compilation, an UndeclaredVariableWarning is raised. All other tests, however, did not change their result after loading this inbox commit.
This behavior leads me to the question of whether it is justified to derive UndeclaredVariableWarning from Warning, given its #defaultAction implementation making it behave like a Notification.
Alternatively, what could we do else if it is not acceptable to interrupt a test once it raises a warning? As I proposed somewhere else, we could catch UnhandledError instead of all these classes eventually open a debugger, but Marcel told me that this would be an implementation detail of the EHS and should not be exposed. Still, I cannot imagine any other way to write an exception handler that detects whether an exception will "raise a problem" eventually or whether it won't. Hmm ... What do you think? :-)
Best,
Christoph
http://www.hpi.de/ ________________________________ Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von Fabio Niephaus <lists at fniephaus.com> Gesendet: Donnerstag, 24. September 2020 10:50:44 An: squeak-dev at lists.squeakfoundation.org Betreff: Re: [squeak-dev] The Inbox: SUnit-ct.129.mcz
Hi Christoph,
If you run all tests in your image without and with your change, does the number of test failures change?
Fabio
On Thu, Sep 24, 2020 at 10:42 AM <commits at source.squeak.org> wrote:
Christoph Thiede uploaded a new version of SUnit to project The Inbox: http://source.squeak.org/inbox/SUnit-ct.129.mcz
==================== Summary ====================
Name: SUnit-ct.129 Author: ct Time: 24 September 2020, 10:42:52.868426 am UUID: 92e68d23-8472-5d48-96d3-8435bd56ac14 Ancestors: SUnit-pre.122
Proposal: Catch warnings and halts in test case execution as well as Errors.
Catching (Error, Warning, Halt) is a common pattern to be (relatively) sure that no debugger will occur during an operation. For related usages, see Morph >> #fullBounds, WorldState >> #displayWorldSafely:, and many other places. IMO it is no desired behavior that the whole test execution, i.e. in a TestRunner, is interrupted because any method under test contains a halt or raises a DeprecationWarning, for example. Instead, the test should be listed as red.
For a similar discussion, see https://github.com/hpi-swa/smalltalkCI/issues/470. I believe we already had talked about this on squeak-dev, but if I remember correctly, I cannot find the thread again.
=============== Diff against SUnit-pre.122 ===============
Item was changed: ----- Method: TestCase>>timeout:after: (in category 'private') ----- timeout: aBlock after: seconds "Evaluate the argument block. Time out if the evaluation is not complete after the given number of seconds. Handle the situation that a timeout may occur after a failure (during debug)"
| theProcess delay watchdog |
"the block will be executed in the current process" theProcess := Processor activeProcess. delay := Delay forSeconds: seconds.
"make a watchdog process" watchdog := [ delay wait. "wait for timeout or completion" theProcess ifNotNil:[ theProcess signalException: (TestFailure new messageText: 'Test timed out') ] ] newProcess.
"Watchdog needs to run at high priority to do its job (but not at timing priority)" watchdog priority: Processor timingPriority-1.
"catch the timeout signal" watchdog resume. "start up the watchdog"
- ^[aBlock on: TestFailure, (Error, Warning, Halt) do: [:ex|
- ^[aBlock on: TestFailure, Error, Halt do:[:ex|
theProcess := nil. ex pass. ]] ensure:[ "evaluate the receiver" theProcess := nil. "it has completed, so ..." delay delaySemaphore signal. "arrange for the watchdog to exit" ]!
Item was added:
- ----- Method: TestResult class>>exAllErrors (in category 'exceptions') -----
- exAllErrors
- ^ self exError, Warning, Halt
- !
Item was changed: ----- Method: TestResult>>runCase: (in category 'running') ----- runCase: aTestCase
| testCasePassed timeToRun | testCasePassed := true.
[timeToRun := [aTestCase runCase] timeToRunWithoutGC] on: self class failure do: [:signal | failures add: aTestCase. testCasePassed := false. signal return: false]
- on: self class exAllErrors
- on: self class error
do: [:signal | errors add: aTestCase. testCasePassed := false. signal return: false].
testCasePassed ifTrue: [passed add: aTestCase]. self durations at: aTestCase put: timeToRun.!
squeak-dev@lists.squeakfoundation.org