Closed Bug 407768 Opened 12 years ago Closed 12 years ago
Minotaur has encoding issues in test-output
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.
Sorry, but, what is Minotaur?
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.
-->Axel reassign if it doesn't belong to you..
Assignee: nobody → l10n
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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 18.104.22.168 ja-JP, 3b2 zh-CN, 22.214.171.124 en-US, and 3.0b2 en-US. UTF-8 characters are now properly outputted.
Attachment #296051 - Flags: review?(rcampbell)
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 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.
Ok, mis-clicked. Sorry for the spam. This is the correct patch.
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.
Patch checked in on trunk --> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Verified fixed using minotaur and ja/zh-CN/zh-TW builds and checked the test-output.xml thanks clint !
Status: RESOLVED → VERIFIED
Component: Testing → Minotaur
Product: Core → Testing
QA Contact: testing → minotaur
You need to log in before you can comment on or make changes to this bug.