Closed
Bug 489728
Opened 15 years ago
Closed 12 years ago
(TUnit) TestTXMgr.cpp printf()s need update
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
People
(Reporter: sgautherie, Assigned: capella)
References
()
Details
Attachments
(4 files, 7 obsolete files)
(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•15 years ago
|
Whiteboard: [good first bug]
Reporter | ||
Updated•15 years ago
|
Assignee | ||
Comment 1•12 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•12 years ago
|
Assignee: nobody → markcapella
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
As mentioned in comment #1 ...
Attachment #599443 -
Flags: feedback?(sgautherie.bz)
Reporter | ||
Comment 3•12 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•12 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•12 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•12 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•12 years ago
|
||
Attachment #599443 -
Attachment is obsolete: true
Attachment #599483 -
Flags: feedback?(sgautherie.bz)
Attachment #599443 -
Flags: feedback?(sgautherie.bz)
Reporter | ||
Comment 8•12 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•12 years ago
|
||
Attachment #599483 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 years ago
|
||
Patch / Diff file, (no changes for TEST FAILS yet)
Attachment #599551 -
Flags: review?(sgautherie.bz)
Reporter | ||
Comment 11•12 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•12 years ago
|
Attachment #599443 -
Attachment description: First Try → Remove ENABLE_DEBUG_PRINTFS, Use 'TEST-UNEXPECTED-FAIL'
Reporter | ||
Updated•12 years ago
|
Attachment #599551 -
Attachment description: Code DIFF File → Use passed()
Reporter | ||
Comment 12•12 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•12 years ago
|
Attachment #599483 -
Attachment description: Sample before / after program output → Use passed(), before / after output log
Reporter | ||
Updated•12 years ago
|
Attachment #599550 -
Attachment description: Before / After Output → Use passed(), before / after output log (v2)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #599550 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
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•12 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•12 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•12 years ago
|
Attachment #599647 -
Attachment description: Use passed(v3) DIFF → Use passed() (v3)
Reporter | ||
Updated•12 years ago
|
Attachment #599551 -
Attachment description: Use passed() → Use passed() (v2)
Assignee | ||
Comment 17•12 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•12 years ago
|
||
Reporter | ||
Comment 19•12 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•12 years ago
|
||
Assignee | ||
Comment 21•12 years ago
|
||
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•12 years ago
|
Attachment #599913 -
Attachment description: After Output Log → After Output Log (v4)
Reporter | ||
Comment 22•12 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•12 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 24•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
||
Attachment #601176 -
Attachment is obsolete: true
Attachment #601205 -
Flags: checkin?(ehsan)
Reporter | ||
Comment 28•12 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•12 years ago
|
||
Did you fix the patch or do I need to re-do it yet?
Reporter | ||
Comment 30•12 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•12 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•12 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•12 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 → ---
Comment 33•12 years ago
|
||
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]]
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•