Closed Bug 407768 Opened 12 years ago Closed 12 years ago

Minotaur has encoding issues in test-output.xml

Categories

(Testing Graveyard :: Minotaur, defect, P2, blocker)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Pike, Assigned: cmtalbert)

Details

Attachments

(1 file, 3 obsolete files)

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?
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
Status: NEW → ASSIGNED
Attached patch Patch to fix output mechanism (obsolete) — Splinter Review
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)
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+
Attached patch Patch addressing comments (obsolete) — Splinter 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+
Ok, mis-clicked. Sorry for the spam.  This is the correct patch.
Attachment #296238 - Attachment is obsolete: true
Attachment #296239 - Flags: review+
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+
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
Flags: blocking1.9+
Product: Core → Testing
QA Contact: testing → minotaur
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.