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

RESOLVED FIXED

Status

()

Core
Editor
--
minor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: capella)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

8 years ago
(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.
(Reporter)

Updated

8 years ago
Whiteboard: [good first bug]
(Reporter)

Updated

8 years ago
Blocks: 443329
(Reporter)

Updated

8 years ago
Blocks: 490255
No longer blocks: 443329
(Assignee)

Comment 1

6 years ago
   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?
(Assignee)

Updated

6 years ago
Assignee: nobody → markcapella
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 599443 [details] [diff] [review]
Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL'


   As mentioned in comment #1 ...
Attachment #599443 - Flags: feedback?(sgautherie.bz)
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
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...)
(Reporter)

Comment 5

6 years ago
(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
(Assignee)

Comment 6

6 years ago
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
(Assignee)

Comment 7

6 years ago
Created attachment 599483 [details]
Use passed(), before / after output log
Attachment #599443 - Attachment is obsolete: true
Attachment #599483 - Flags: feedback?(sgautherie.bz)
Attachment #599443 - Flags: feedback?(sgautherie.bz)
(Reporter)

Comment 8

6 years ago
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.
Attachment #599483 - Flags: feedback?(sgautherie.bz)
(Assignee)

Comment 9

6 years ago
Created attachment 599550 [details]
Use passed(), before / after output log (v2)
Attachment #599483 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 599551 [details] [diff] [review]
Use passed() (v2)

Patch / Diff file, (no changes for TEST FAILS yet)
Attachment #599551 - Flags: review?(sgautherie.bz)
(Reporter)

Comment 11

6 years ago
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.
(Reporter)

Updated

6 years ago
Attachment #599443 - Attachment description: First Try → Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL'
(Reporter)

Updated

6 years ago
Attachment #599551 - Attachment description: Code DIFF File → Use passed()
(Reporter)

Comment 12

6 years ago
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.
(Reporter)

Updated

6 years ago
Attachment #599483 - Attachment description: Sample before / after program output → Use passed(), before / after output log
(Reporter)

Updated

6 years ago
Attachment #599550 - Attachment description: Before / After Output → Use passed(), before / after output log (v2)
(Assignee)

Comment 13

6 years ago
Created attachment 599643 [details]
Use passed(), before / after output log (v3)
Attachment #599550 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
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 ...
Attachment #599551 - Attachment is obsolete: true
Attachment #599647 - Flags: review?(sgautherie.bz)
Attachment #599551 - Flags: review?(sgautherie.bz)
(Reporter)

Comment 15

6 years ago
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.
(Reporter)

Comment 16

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

Just attach bare before and after outputs separately.
Attachment #599643 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #599647 - Attachment description: Use passed(v3) DIFF → Use passed() (v3)
(Reporter)

Updated

6 years ago
Attachment #599551 - Attachment description: Use passed() → Use passed() (v2)
(Assignee)

Comment 17

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

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

Comment 18

6 years ago
Created attachment 599909 [details]
Before Output Log
(Reporter)

Comment 19

6 years ago
(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).
(Assignee)

Comment 20

6 years ago
Created attachment 599913 [details]
After Output Log (v4)
(Assignee)

Comment 21

6 years ago
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 ...
Attachment #599647 - Attachment is obsolete: true
Attachment #599917 - Flags: feedback?(sgautherie.bz)
Attachment #599647 - Flags: review?(sgautherie.bz)
(Reporter)

Updated

6 years ago
Attachment #599913 - Attachment description: After Output Log → After Output Log (v4)
(Reporter)

Comment 22

6 years ago
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.
Attachment #599917 - Flags: review?(peterv)
Attachment #599917 - Flags: feedback?(sgautherie.bz)
Attachment #599917 - Flags: feedback+
(Assignee)

Comment 23

6 years ago
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
Attachment #599917 - Flags: review?(peterv) → review?(ehsan)
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.
Attachment #599917 - Flags: review?(ehsan) → review+
(Assignee)

Comment 25

6 years ago
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....
Attachment #601176 - Flags: checkin?(ehsan)
(Reporter)

Comment 26

6 years ago
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.
Attachment #601176 - Flags: checkin?(ehsan)
(Assignee)

Comment 27

6 years ago
Created attachment 601205 [details] [diff] [review]
Use passed() (Bv6)
[Checked in: See comment 28+29]
Attachment #601176 - Attachment is obsolete: true
Attachment #601205 - Flags: checkin?(ehsan)
(Reporter)

Comment 28

6 years ago
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.
Attachment #601205 - Attachment description: Use passed() (v6) → Use passed() (Bv6) [Checked in: See comment 28]
Attachment #601205 - Flags: checkin?(ehsan) → checkin+
(Assignee)

Comment 29

6 years ago
Did you fix the patch or do I need to re-do it yet?
(Reporter)

Comment 30

6 years ago
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 '"'.
Attachment #601205 - Attachment description: Use passed() (Bv6) [Checked in: See comment 28] → Use passed() (Bv6) [Checked in: See comment 28+29]
(Reporter)

Comment 31

6 years ago
(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).
Flags: in-testsuite+
Target Milestone: --- → mozilla13
(Assignee)

Comment 32

6 years ago
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...
(Reporter)

Updated

6 years ago
Assignee: markcapella → nobody
Severity: normal → minor
Status: ASSIGNED → NEW
Whiteboard: [good first bug] → [good first bug][mentor=sgautherie][lang=c++] [fixed in mozilla13: Bv6]]
Target Milestone: mozilla13 → ---
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!
Assignee: nobody → markcapella
Whiteboard: [good first bug][mentor=sgautherie][lang=c++] [fixed in mozilla13: Bv6]]
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.