407768 - Minotaur has encoding issues in test-output.xml

Status: VERIFIED FIXED

(Testing Graveyard :: Minotaur, defect, P2)

Description

[Axel Hecht [:Pike]]

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.

Comment 1

[Damon Sicore (:damons)]

Sorry, but, what is Minotaur?

Comment 2

[Axel Hecht [:Pike]]

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

[Mike Schroepfer]

-->Axel reassign if it doesn't belong to you..

Comment 4

[Axel Hecht [:Pike]]

Clint, are you working on this? If so, please take this bug?

Comment 5

[cmtalbert]

Yeah, I'm working on this. Reassigning.

Comment 6

[cmtalbert]

Attachment: Patch to fix output mechanism

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.

Comment 7

[Axel Hecht [:Pike]]

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

[Rob Campbell [:rc] (:robcee)]

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.

Comment 9

[cmtalbert]

Attachment: Patch addressing comments

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.

Comment 10

[cmtalbert]

Attachment: The CORRECT patch with the changes

Ok, mis-clicked. Sorry for the spam.  This is the correct patch.

Comment 11

[cmtalbert]

Attachment: Patch without the extra flush() statement

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.

Comment 12

[cmtalbert]

Patch checked in on trunk --> FIXED

Comment 13

[Carsten Book [:Tomcat]]

Verified fixed using minotaur and ja/zh-CN/zh-TW builds and checked the test-output.xml thanks clint !