Closed
Bug 407768
Opened 15 years ago
Closed 15 years ago
Minotaur has encoding issues in test-output.xml
Categories
(Testing Graveyard :: Minotaur, defect, P2)
Testing Graveyard
Minotaur
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: Pike, Assigned: cmtalbert)
Details
Attachments
(1 file, 3 obsolete files)
2.39 KB,
patch
|
cmtalbert
:
review+
|
Details | Diff | Splinter Review |
The current test-output.xml's for ja and zh-CN have encoding issues. Those block the use of Minotaur for testing, as this could be data loss as well.
Flags: blocking1.9?
Comment 1•15 years ago
|
||
Sorry, but, what is Minotaur?
Reporter | ||
Comment 2•15 years ago
|
||
http://quality.mozilla.org/en/projects/automation/minotaur is supposed to help us automate testing of productization parts like search and rss readers. Having this or not having this has a major impact on the release work, and test-output.xml is one of the central files to be checked.
Comment 3•15 years ago
|
||
-->Axel reassign if it doesn't belong to you..
Assignee: nobody → l10n
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Reporter | ||
Comment 4•15 years ago
|
||
Clint, are you working on this? If so, please take this bug?
Yeah, I'm working on this. Reassigning.
Assignee: l10n → ctalbert
Was not using a nsIConverterOutputStream to ensure that UTF-8 characters would be written out properly to the output file. Tested this with 2.0.0.11 ja-JP, 3b2 zh-CN, 2.0.0.11 en-US, and 3.0b2 en-US. UTF-8 characters are now properly outputted.
Attachment #296051 -
Flags: review?(rcampbell)
Reporter | ||
Comment 7•15 years ago
|
||
Only technical concerns on the patch here: Both the 1.8 and the trunk versions of converter output stream close the underlying stream, see http://mxr.mozilla.org/mozilla1.8/source/intl/uconv/src/nsConverterOutputStream.cpp#160 so I think that the gOutFile.close() shouldn't be necessary. And thus, gOutFile wouldn't need to be global anymore.
Comment 8•15 years ago
|
||
Comment on attachment 296051 [details] [diff] [review] Patch to fix output mechanism Thanks for the review Axel! You made my job easy. It looks like you don't have to worry about closing gOutFile if you close gConvStream, so r+ with those changes in mind.
Attachment #296051 -
Flags: review?(rcampbell) → review+
Thanks for that catch. After I submitted it, I started thinking about the same thing this morning. This patch removes gOutFile as a global and the extra gOutFile.close. Carrying Rob's review forward.
Attachment #296051 -
Attachment is obsolete: true
Attachment #296238 -
Flags: review+
Assignee | ||
Comment 10•15 years ago
|
||
Ok, mis-clicked. Sorry for the spam. This is the correct patch.
Attachment #296238 -
Attachment is obsolete: true
Attachment #296239 -
Flags: review+
Assignee | ||
Comment 11•15 years ago
|
||
Per discussion on IRC, Axel and I decided that the flush() before the close() was extra as Close() is defined to flush the buffer: dynamis joined the chat room. [3:44pm] Pike: ctalbert: i think you don't need the flush() either [3:44pm] dynamis left the chat room. (Client exited) [3:45pm] dynamis joined the chat room. [3:45pm] ctalbert: yeah, close does a flush [3:45pm] dynamis left the chat room. (Client exited) [3:45pm] bretr joined the chat room. [3:45pm] ctalbert: should one depend on that being handled under the covers? [3:46pm] Mic joined the chat room. [3:47pm] Pike: ctalbert: nsIOutputStream defines close to flush the output stream [3:47pm] ctalbert: Ah, ok then. [3:47pm] ctalbert: r3 coming up [3:48pm] Pike: wait [3:50pm] ctalbert: Pike: It says this: http://mxr.mozilla.org/mozilla/source/xpcom/io/nsIOutputStream.idl#97 [3:50pm] ctalbert: So I think you're right [3:50pm] Pike: ah, ok. So, nsIUnicharOutputStream defines close() to close the underlying stream, and the underlying stream is a nsIOutputStream, which is defined to flush() [3:50pm] ctalbert: right [3:51pm] Pike: so, yeah, not flush() required by the interfaces, so we'd not rely on implementation detail here. Not that I'd be tooo worried about doing that, even [3:51pm] ctalbert: true. Thanks for looking it over.
Attachment #296239 -
Attachment is obsolete: true
Attachment #296241 -
Flags: review+
Assignee | ||
Comment 12•15 years ago
|
||
Patch checked in on trunk --> FIXED
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 13•14 years ago
|
||
Verified fixed using minotaur and ja/zh-CN/zh-TW builds and checked the test-output.xml thanks clint !
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Component: Testing → Minotaur
Flags: blocking1.9+
Product: Core → Testing
QA Contact: testing → minotaur
Updated•4 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•