Make cctest test-debug.cc more accurate

Make cctest test-debug.cc more accurate

Committed: http://code.google.com/p/v8/source/detail?r=3044

[Peter Rybin, 2009-09-29 20:16:50]

Hi Soren

Before I change running variable I'd like to clean up our cctest tests. I made
necessary improvements to have tests still passing after my change.

In particular "message.GetEvent() == v8::Break" seem to be unreliable check. I
replaced it with a more precise condition.

Peter

[Søren Thygesen Gjesse, 2009-09-30 08:34:48]

LGTM, with comments

The JSON protocol debugger tests are definitely in need of an overhaul. If you
can think of a better (more generic) way of testing different sequences it would
be great.

http://codereview.chromium.org/243036/diff/1002/2002
File test/cctest/test-debug.cc (right):

http://codereview.chromium.org/243036/diff/1002/2002#newcode3556
Line 3556: bool IsExceptionOrBreakEventMessage(char *message) {
I think we should have an IsBreakEventMessage and a IsExceptionEventMessage as
well, and use the most precise expectation in the tests. I don't remember all
the tests, but are we testing exceptions that much?

http://codereview.chromium.org/243036/diff/1002/2002#newcode3607
Line 3607: printf("%s\n", print_buffer);
Maybe get rid of these printf's while at it.

http://codereview.chromium.org/243036/diff/1002/2002#newcode3962
Line 3962: // Is break_template a prefix of the message?
This comment is somewhat misleading now.

http://codereview.chromium.org/243036/diff/1002/2002#newcode4043
Line 4043: // 1:Set breakpoint in cat() (must have id 1).
must have -> will get

http://codereview.chromium.org/243036/diff/1002/2002#newcode4045
Line 4045: // 2:Set breakpoint in dog() (must have id 2).
Ditto.

http://codereview.chromium.org/243036/diff/1002/2002#newcode4055
Line 4055: // must have hit breakpoint #1.
Uppercase first letter in comment here and 5 places below.

http://codereview.chromium.org/243036/diff/1002/2002#newcode4067
Line 4067: // must have result 105.
Maybe change the range of values used in the test so that they do not collide
with the sequence numbers, to avoid confusion.

http://codereview.chromium.org/243036/diff/1002/2002#newcode4851
Line 4851: if (IsExceptionOrBreakEventMessage(print_buffer)) {
I don't understand why the check message.GetEvent() == v8::Break is not reliable
here.

[Peter Rybin, 2009-10-05 15:01:05]

http://codereview.chromium.org/243036/diff/1002/2002
File test/cctest/test-debug.cc (right):

http://codereview.chromium.org/243036/diff/1002/2002#newcode3556
Line 3556: bool IsExceptionOrBreakEventMessage(char *message) {
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> I think we should have an IsBreakEventMessage and a IsExceptionEventMessage as
> well, and use the most precise expectation in the tests. I don't remember all
> the tests, but are we testing exceptions that much?

My reasoning here was to send "continue" in response to any stopping event,
because otherwise we may just hang indefinitely.

Done

http://codereview.chromium.org/243036/diff/1002/2002#newcode3607
Line 3607: printf("%s\n", print_buffer);
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> Maybe get rid of these printf's while at it.

Done.

http://codereview.chromium.org/243036/diff/1002/2002#newcode3962
Line 3962: // Is break_template a prefix of the message?
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> This comment is somewhat misleading now.

Don't know whether we need to write something instead now

Done

http://codereview.chromium.org/243036/diff/1002/2002#newcode4043
Line 4043: // 1:Set breakpoint in cat() (must have id 1).
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> must have -> will get

Well, I meant I am not 100% sure it will be #1. I'd rather wanted to imply we'd
better have assert here.

Done

http://codereview.chromium.org/243036/diff/1002/2002#newcode4045
Line 4045: // 2:Set breakpoint in dog() (must have id 2).
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/243036/diff/1002/2002#newcode4055
Line 4055: // must have hit breakpoint #1.
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> Uppercase first letter in comment here and 5 places below.

Done.

http://codereview.chromium.org/243036/diff/1002/2002#newcode4067
Line 4067: // must have result 105.
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> Maybe change the range of values used in the test so that they do not collide
> with the sequence numbers, to avoid confusion.

Originally all 3 results were "104". I was close to idea that it was intentional
and had some sense.

Done

http://codereview.chromium.org/243036/diff/1002/2002#newcode4851
Line 4851: if (IsExceptionOrBreakEventMessage(print_buffer)) {
On 2009/09/30 08:34:48, Søren Gjesse wrote:
> I don't understand why the check message.GetEvent() == v8::Break is not
reliable
> here.

see debug.cc:2316 ("Return the result.")

[Søren Thygesen Gjesse, 2009-10-07 14:42:54]

LGTM

http://codereview.chromium.org/243036/diff/4001/4002
File test/cctest/test-debug.cc (right):

http://codereview.chromium.org/243036/diff/4001/4002#newcode4394
Line 4394: if (IsExceptionEventMessage(print_buffer)) {
This does not match the comment below. Can´t this handler just always send a
continue command?

[Peter Rybin, 2009-10-07 15:17:07]

http://codereview.chromium.org/243036/diff/4001/4002
File test/cctest/test-debug.cc (right):

http://codereview.chromium.org/243036/diff/4001/4002#newcode4394
Line 4394: if (IsExceptionEventMessage(print_buffer)) {
On 2009/10/07 14:42:54, Søren Gjesse wrote:
> This does not match the comment below. Can´t this handler just always send a
> continue command?

I thought you wanted to "use the most precise expectation in
the tests" :)

Actually continue command has its own response message which will feed us back
(is it called Larse effect?) and we'll get into infinite loop.