[rfc][icedtea-web] Console Output Encoding Fix

[Jacob Wisor]

On 07/16/2014 08:06 PM, Jie Kang wrote:
> ----- Original Message -----
>> On 07/16/2014 06:09 PM, Jie Kang wrote:
>>
>> Overall the patch looks good. One last nit though:
>>
>>   > diff --git a/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>>   > b/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>>   > --- a/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>>   > +++ b/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>>   > […]
>>   > +    public ByteArrayOutputStream getByteArrayOutputStream() {
>>   > +        return this.byteArrayOutputStream;
>>   > +    }
>>
>> Do we really want to expose this internal ByteArrayOutputStream instance? I
>> cannot think of any good reason now. Actually, it is a helper for
>> TeeOutputStream to be able to flush() on every new line. What if a caller
>> calls
>> ByteArrayOutputStream.reset() before TeeOutputStream has flushed? Then
>> possibly
>> data could get lost. Because of this, in any case, this method should return
>> a
>> clone of this.byteArrayOutputStream.
>
>
> Hmm good catch on the reset use case.
> I was having a lot of trouble figuring out how to properly unit test TeeOutputStream which is a sign that the whole class needs to be rewritten but at the same time I have yet to think of an acceptable reimplementation.

Hmm, I do not think TeeOutputStream needs a rewrite. Except that it perhaps may 
have been better to make it extend from a custom FilterOutputStream and then 
write to a PrintStream.

If you are having difficulties writing a test for TeeOutputStream then you 
should probably take a step back and think again about *what* a test is actually 
testing for or rather verifying. Actually, it is not that difficult, it is 
fairly easy in this case.

 > So this might be considered a temporary patch that resolves the bug before a 
rework of the whole class. And also we must consider that the backport to 1.5 
should probably not have a rewrite of the class, which will use this patch.
>
> In terms of the problem with unit testing, we need to make sure calls to TeeOutputStream result in successful writes to the ByteOutputStream. However, given the current implementation there really isn't any way that I can see to validate that ByteOutputStream contains the correct write information without either exposing something or rewriting the class. I decided the getter was the best solution for the moment. It's also possible to expose flushLog() and have it return the flushed contents, but this sidesteps the PrintStream and doesn't feel as good to me.

No, a reference to TeeOutputStream.byteArrayOutputStream is not required for a 
test. We should not be even bothering about it because it is not 
TeeOutputStream's /output/.

> I've made it return a copy and also set the method to be protected since it's really only used for testing. Just to make it clear, I also really dislike this approach so if you have any suggestions I will be happy to hear them.

My advice is, as always with tests; test for the output depending on input, not 
a unit's or module's internal structure.
TeeOutputStream's output are its print() and println() methods. You can read 
this output with a PipedInputStream. Verify the input you get from 
PipedInputStream, not TeeOutputStream's internal structure. Then there is also 
no need to expose TeeOutputStream.byteArrayOutputStream.

Jacob