Last Comment Bug 489728 - (TUnit) TestTXMgr.cpp printf()s need update
: (TUnit) TestTXMgr.cpp printf()s need update
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- minor (vote)
: ---
Assigned To: Mark Capella [:capella]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 369034
Blocks: 490255 117440
  Show dependency treegraph
 
Reported: 2009-04-22 21:10 PDT by Serge Gautherie (:sgautherie)
Modified: 2012-04-03 21:30 PDT (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL' (139.41 KB, patch)
2012-02-21 18:32 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Use passed(), before / after output log (14.67 KB, text/plain)
2012-02-21 22:17 PST, Mark Capella [:capella]
no flags Details
Use passed(), before / after output log (v2) (14.12 KB, text/plain)
2012-02-22 03:51 PST, Mark Capella [:capella]
no flags Details
Use passed() (v2) (43.86 KB, patch)
2012-02-22 03:53 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Use passed(), before / after output log (v3) (14.12 KB, text/plain)
2012-02-22 09:09 PST, Mark Capella [:capella]
no flags Details
Use passed() (v3) (41.73 KB, patch)
2012-02-22 09:15 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Before Output Log (6.90 KB, text/plain)
2012-02-23 00:42 PST, Mark Capella [:capella]
no flags Details
After Output Log (v4) (7.05 KB, text/plain)
2012-02-23 01:07 PST, Mark Capella [:capella]
no flags Details
Use passed() (v4) (40.42 KB, patch)
2012-02-23 01:12 PST, Mark Capella [:capella]
ehsan: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Review
Use passed() (v5) (40.59 KB, patch)
2012-02-27 21:22 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Use passed() (Bv6) [Checked in: See comment 28+29] (40.66 KB, patch)
2012-02-28 01:02 PST, Mark Capella [:capella]
bugzillamozillaorg_serge_20140323: checkin+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2009-04-22 21:10:14 PDT
(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] 2012-02-15 02:40:08 PST
   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] 2012-02-21 18:32:37 PST
Created attachment 599443 [details] [diff] [review]
Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL'


   As mentioned in comment #1 ...
Comment 3 Serge Gautherie (:sgautherie) 2012-02-21 19:23:46 PST
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) 2012-02-21 19:28:09 PST
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) 2012-02-21 19:45:26 PST
(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] 2012-02-21 19:58:36 PST
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] 2012-02-21 22:17:58 PST
Created attachment 599483 [details]
Use passed(), before / after output log
Comment 8 Serge Gautherie (:sgautherie) 2012-02-22 03:22:35 PST
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] 2012-02-22 03:51:05 PST
Created attachment 599550 [details]
Use passed(), before / after output log (v2)
Comment 10 Mark Capella [:capella] 2012-02-22 03:53:26 PST
Created attachment 599551 [details] [diff] [review]
Use passed() (v2)

Patch / Diff file, (no changes for TEST FAILS yet)
Comment 11 Serge Gautherie (:sgautherie) 2012-02-22 05:31:25 PST
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) 2012-02-22 05:37:09 PST
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] 2012-02-22 09:09:05 PST
Created attachment 599643 [details]
Use passed(), before / after output log (v3)
Comment 14 Mark Capella [:capella] 2012-02-22 09:15:08 PST
Created attachment 599647 [details] [diff] [review]
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) 2012-02-23 00:09:37 PST
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) 2012-02-23 00:11:08 PST
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] 2012-02-23 00:29:13 PST
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] 2012-02-23 00:42:41 PST
Created attachment 599909 [details]
Before Output Log
Comment 19 Serge Gautherie (:sgautherie) 2012-02-23 00:52:49 PST
(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] 2012-02-23 01:07:46 PST
Created attachment 599913 [details]
After Output Log (v4)
Comment 21 Mark Capella [:capella] 2012-02-23 01:12:33 PST
Created attachment 599917 [details] [diff] [review]
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) 2012-02-23 01:29:41 PST
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] 2012-02-25 05:10:15 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-02-27 19:35:16 PST
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] 2012-02-27 21:22:07 PST
Created attachment 601176 [details] [diff] [review]
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) 2012-02-28 00:51:55 PST
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] 2012-02-28 01:02:04 PST
Created attachment 601205 [details] [diff] [review]
Use passed() (Bv6)
[Checked in: See comment 28+29]
Comment 28 Serge Gautherie (:sgautherie) 2012-02-28 01:23:10 PST
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] 2012-02-28 01:27:34 PST
Did you fix the patch or do I need to re-do it yet?
Comment 30 Serge Gautherie (:sgautherie) 2012-02-28 01:33:43 PST
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) 2012-02-28 02:23:23 PST
(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] 2012-02-29 17:37:08 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-03 21:26:15 PDT
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!

Note You need to log in before you can comment on or make changes to this bug.