489728 - (TUnit) TestTXMgr.cpp printf()s need update

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Serge Gautherie (:sgautherie)]

(Let's fix bug 369034 first, then)

Ideas:
*Get rid (= replace) |ENABLE_DEBUG_PRINTFS + printf()|.
*Replace "ERROR"/etc by "TEST-PASS"/"TEST-UNEXPECTED-FAIL".
 And/Or use NS_ERROR() etc.

Comment 1

[Mark Capella [:capella]]

   Looking at [good first bugs] ... found this old / seemingly simple one ... 

   Removing (4) references to ENABLE_DEBUG_PRINTFS and associated printf()'s is easy enough...

   Also, changing (417) uses of ERROR: text to be TEST-UNEXPECTED-FAIL: text is also easy ...

   The use of NS_ERROR("*** Test ***"); doesn't seem to generate executable code so I guess I'm missing something in the build. What's the expected benefit to using this?

Comment 2

[Mark Capella [:capella]]

Attachment: Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL'

As mentioned in comment #1 ...

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 599443 [details] [diff] [review]
Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL'

Review of attachment 599443 [details] [diff] [review]:
-----------------------------------------------------------------

I checked beginning and end of patch only.
If there are other types of modifications than the 3 I commented about, please write about it.

::: editor/txmgr/tests/TestTXMgr.cpp
@@ -50,5 @@
>  static PRInt32 *sUndoOrderArr        = 0;
>  static PRInt32 sRedoCount            = 0;
>  static PRInt32 *sRedoOrderArr        = 0;
>  
> -// #define ENABLE_DEBUG_PRINTFS 1

I'm not sure what my idea was back then: let's move this part to a second/later patch.

http://mxr.mozilla.org/comm-central/search?string=ENABLE_DEBUG_PRINTFS&case=on

@@ +475,5 @@
>      // This is done on purpose since we want to crash if the order array is out
>      // of date.
>      //
>      if (sDestructorOrderArr && mVal != sDestructorOrderArr[sDestructorCount]) {
> +      printf("TEST-UNEXPECTED-FAIL: ~SimpleTransaction expected %d got %d.\n",

I think the idea would be to use fail() from
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/tests/TestHarness.h

Could you try that?

@@ +4600,5 @@
>     *******************************************************************/
>  
>    printf("\n-----------------------------------------------------\n");
>    printf("- Simple Transaction Stress Test:\n");
> +  printf("-----------------------------------------------------\n\n");

Why the added '\n'?
If needed, please post an output example of before and after.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 599443 [details] [diff] [review]
Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL'

>   printf("passed\n");

I think the idea would be to use passed() from
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/tests/TestHarness.h

Could you try that?

(Do a separate patch: issues are trivial, but there are so many occurrences of each...)

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to Mark Capella [:capella] from comment #1)
>    The use of NS_ERROR("*** Test ***"); doesn't seem to generate executable
> code so I guess I'm missing something in the build. What's the expected
> benefit to using this?

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#55
NS_ERROR() does nothing in Opt build, but is useful in Debug builds.

Nit: Can you not indent your comments? It looks odd.


(In reply to Serge Gautherie (:sgautherie) from comment #3)
> I think the idea would be to use fail() from
> http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/tests/TestHarness.h

And make use of
http://mxr.mozilla.org/comm-central/search?string=gFailCount&case=on

Comment 6

[Mark Capella [:capella]]

First pass in progress: leave ENABLE_DEBUG_PRINTFS alone, dropping any added '\n' ... it just looked nicer to me, changing "passed" printing to passed() function in this first patch, addressing "fail" printing using fail() function in next patch, and I'll keep the comments more "vanilla"... I've written magazine articles and a book, so paragraph indenting / spacing comes natural to me :-P 

Back with patch#1 ASAP -- mark

Comment 7

[Mark Capella [:capella]]

Attachment: Use passed(), before / after output log

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 599483 [details]
Use passed(), before / after output log

This looks promising :-)

Nits:
*Keep |printf("\n"| before headers, ftb.
*Remove " ... " too, and '.' at end of passed() lines.
*Remove 3 remaining "TEST-PASS | passed".

Please attach old output, new output and code diff too.

Comment 9

[Mark Capella [:capella]]

Attachment: Use passed(), before / after output log (v2)

Comment 10

[Mark Capella [:capella]]

Attachment: Use passed() (v2)

Patch / Diff file, (no changes for TEST FAILS yet)

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 599551 [details] [diff] [review]
Use passed() (v2)

Review of attachment 599551 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/txmgr/tests/TestTXMgr.cpp
@@ +51,5 @@
>  static PRInt32 sRedoCount            = 0;
>  static PRInt32 *sRedoOrderArr        = 0;
>  
>  // #define ENABLE_DEBUG_PRINTFS 1
> +#define MAX_BUFFER_SIZE 512

No need for this define: merge it.

@@ +53,5 @@
>  
>  // #define ENABLE_DEBUG_PRINTFS 1
> +#define MAX_BUFFER_SIZE 512
> +
> +static char tmpbuf[MAX_BUFFER_SIZE];

Call it something more descriptive, like 'logMessage'.

@@ +489,5 @@
>      ++sDestructorCount;
>  
>  #ifdef ENABLE_DEBUG_PRINTFS
> +    sprintf(tmpbuf, "~SimpleTransaction: %d - 0x%.8x", mVal, (PRInt32)this);
> +    passed(tmpbuf);

Actually, it seems a good time to make passed() be similar to fail() in
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/tests/TestHarness.h

Can you try that?

@@ +4492,5 @@
>      // Trivial feedback not to let the user think the test is stuck.
>      if (NS_UNLIKELY(j % 100 == 0))
>        printf("%i ", j);
>    } // for, iterations.
> +    printf("\n");

This one should stay, ftb.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 599551 [details] [diff] [review]
Use passed() (v2)

Review of attachment 599551 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/txmgr/tests/TestTXMgr.cpp
@@ +511,5 @@
>      }
>  
>      ++sDoCount;
>  
>  #ifdef ENABLE_DEBUG_PRINTFS

And do not modify these, wrt this patch.

Comment 13

[Mark Capella [:capella]]

Attachment: Use passed(), before / after output log (v3)

Comment 14

[Mark Capella [:capella]]

Attachment: Use passed() (v3)

Removed MAX_BUFFER_SIZE VAR, removed tmpbuf char array, left ENABLE_DEBUG_PRINTFS logic as was prior to all changes, made passed() to be similar to fail() in TestHarness.h ...

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 599647 [details] [diff] [review]
Use passed() (v3)

Review of attachment 599647 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/TestHarness.h
@@ +93,5 @@
>    ++gFailCount;
>  }
>  
>  /**
>   * Prints the given string prepending "TEST-PASS | " for the benefit of

Update comment too.

@@ +112,1 @@
>  }

Nit: Leave the blank line in.

::: editor/txmgr/tests/TestTXMgr.cpp
@@ +52,5 @@
>  static PRInt32 *sRedoOrderArr        = 0;
>  
>  // #define ENABLE_DEBUG_PRINTFS 1
>  
>  PRInt32 sSimpleTestDestructorOrderArr[] = {

Nit: Odd, this block without any change...

@@ +4382,5 @@
>   **/
>  nsresult
>  stress_test(TestTransactionFactory *factory, PRInt32 iterations)
>  {
> +  printf("Stress test of %i iterations (may take a while) ", iterations);

" ... " should stay on these (printf) lines.

@@ +4484,5 @@
>      // Trivial feedback not to let the user think the test is stuck.
>      if (NS_UNLIKELY(j % 100 == 0))
>        printf("%i ", j);
>    } // for, iterations.
> +    printf("\n");

Nit: Wrong indentation.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 599643 [details]
Use passed(), before / after output log (v3)

Just attach bare before and after outputs separately.

Comment 17

[Mark Capella [:capella]]

Re: /Nit: Odd, this block without any change.../

Is this an action item? I'm not sure what you're asking for here.

Comment 18

[Mark Capella [:capella]]

Attachment: Before Output Log

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to Mark Capella [:capella] from comment #17)
> Re: /Nit: Odd, this block without any change.../
> 
> Is this an action item? I'm not sure what you're asking for here.

Just that you check what is going on with your patch:
no change would mean this block should not be included,
included block would mean there should be some change(s).

Comment 20

[Mark Capella [:capella]]

Attachment: After Output Log (v4)

Comment 21

[Mark Capella [:capella]]

Attachment: Use passed() (v4)

I see it now! That >is< odd ... however, it's gone from this DIFF ... after all these revisions my eyes must be tired and I'm starting to miss things ...

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 599917 [details] [diff] [review]
Use passed() (v4)

Review of attachment 599917 [details] [diff] [review]:
-----------------------------------------------------------------

Wait for review before updating this patch :-)

::: xpcom/tests/TestHarness.h
@@ +93,5 @@
>    ++gFailCount;
>  }
>  
>  /**
> + * Prints the given pass message and arguments using printf, prepending

Nit: s/pass/success/.

::: editor/txmgr/tests/TestTXMgr.cpp
@@ +4483,5 @@
>      if (NS_UNLIKELY(j % 100 == 0))
>        printf("%i ", j);
>    } // for, iterations.
>  
> +  printf("\n");

What I meant in comment 11 was to keep "passed" on this (printf) line.

Comment 23

[Mark Capella [:capella]]

Comment on attachment 599917 [details] [diff] [review]
Use passed() (v4)

I changed the review? request to be :ehsan, as I found he's listed as the module owner: https://wiki.mozilla.org/Modules/Core

Comment 24

[(no longer active)]

Comment on attachment 599917 [details] [diff] [review]
Use passed() (v4)

Review of attachment 599917 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, Mark, thanks for your patch!

After you address the comment below, please attach an updated version of the patch and set the checkin? flag on the new attachment.  Thanks!

::: xpcom/tests/TestHarness.h
@@ +93,5 @@
>    ++gFailCount;
>  }
>  
>  /**
> + * Prints the given pass message and arguments using printf, prepending

Please address Serge's comment here.

Comment 25

[Mark Capella [:capella]]

Attachment: Use passed() (v5)

Please note that this is my first patch using Mercurial Queues... please review carefully for beginners mistakes... also... is there a way to/a need to post to TRY server first?

Being cautious....

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 601176 [details] [diff] [review]
Use passed() (v5)

Review of attachment 601176 [details] [diff] [review]:
-----------------------------------------------------------------

Please use a patch comment more like "Bug 489728. (Bv6) Use passed() in TestTXMgr.cpp, Make passed() accept a va_list in TestHarness.h.".
To which you can add "f=sgautherie r=ehsan.".

::: editor/txmgr/tests/TestTXMgr.cpp
@@ +4483,5 @@
>      if (NS_UNLIKELY(j % 100 == 0))
>        printf("%i ", j);
>    } // for, iterations.
>  
> +  printf("\n");

You still miss to re-add "passed" on this (printf) line.

Comment 27

[Mark Capella [:capella]]

Attachment: Use passed() (Bv6) [Checked in: See comment 28+29]

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 601205 [details] [diff] [review]
Use passed() (Bv6)
[Checked in: See comment 28+29]

https://hg.mozilla.org/mozilla-central/rev/c3f06b2e9dfa
Bv6, with fixed comment and code.


>Bug 489728. (Bv6) Use passed() in TestTXMgr.cpp
>Make passed() accept a va_list in TestHarness.h, f=sgautherie r=ehsan

Supposed to usually be all on one line.

>+  printf(passed"\n");
>-  printf("passed\n");

Misplaced '"'.


(In reply to Mark Capella [:capella] from comment #25)
> is there a way to/a need to post to TRY server first?

A way, yes, a need, I don't think so for this patch.

Comment 29

[Mark Capella [:capella]]

Did you fix the patch or do I need to re-do it yet?

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 601205 [details] [diff] [review]
Use passed() (Bv6)
[Checked in: See comment 28+29]

(In reply to Serge Gautherie (:sgautherie) from comment #28)
> https://hg.mozilla.org/mozilla-central/rev/c3f06b2e9dfa
> 
> >+  printf(passed"\n");
> >-  printf("passed\n");
> 
> Misplaced '"'.

Arf, I missed to actually fix it:
https://hg.mozilla.org/mozilla-central/rev/8c8346a7fae1
(Bv6a_fix) Misplaced '"'.

Comment 31

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #30)
> > https://hg.mozilla.org/mozilla-central/rev/c3f06b2e9dfa
> https://hg.mozilla.org/mozilla-central/rev/8c8346a7fae1

https://tbpl.mozilla.org/php/getParsedLog.php?id=9683542&tree=Firefox&full=1
Linux QT mozilla-central build on 2012-02-28 01:31:48 PST for push 8c8346a7fae1

V.Fixed wrt this patch (only).

Comment 32

[Mark Capella [:capella]]

Ok Serge... can we close this one then? Anything that's left that you can think of can be addressed in a new bug ... or if you have additional changes that you'd like done under this same bug, drop it it back to assigned -> nobody to open it to the field...

Comment 33

[(no longer active)]

Serge should file a new bug if he wants to see other changes landed for this test.  We try not to do multiple patches per bug.

Thanks a lot, Mark, for your patch!