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

[Jie Kang]

----- Original Message -----
> 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 wonder why there was a bug this simple... maybe because it wasn't tested?

> 
> > 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.

I think you are missing the test case's purpose here or I am misunderstanding you. The purpose of TeeOutputStream is two-fold.
a) it supers PrintStream and pipes all input to some outputStream given.
b) it also 'pipes' all input to the logger through flushLog(). I use quotes on the word pipe because the way it works is nowhere near as nice as piping.

You seem to be thinking that we should only test a) which assuming PrintStream works, is not very useful.

The bug that is being fixed here is that when TeeOutputStream wrote to the logger through flushLog(), data was lost as it did a byte-by-byte to char assignment to the StringBuffer. This has nothing to do with TeeOutputStream's output to whatever stream is given to it on construction. The output through supering to PrintStream did not have this issue as PrintStream actually works (yay). If you ran a user-test on this with some special character like 'ó' you would note that the stream given to TeeOutputStream contains the correct string while the logging that eventually goes to JavaConsole does not contain the correct string. In this situation it would have been nice to make sure that the logging had worked correctly, which is where unit tests come in.

The way the logging works is it calls OutputController.getLogger.log(message) which stores the message in a queue and then consumes it on it's own accord, which eventually goes to JavaConsole, which has a List of messages recieved that is displayed in the GUI. As you can see here, there is no piping involved which is unfortunate as if it did, unit testing would be as easy as your above solution. We could also add dependencies on OutputController and/or JavaConsole in our tests but that destroys the concept of a unit test. Or we could also consider applying a mocking framework such as mockito but I don't think ITW uses mockito and I am reluctant to suggest adding it.

So... what do we need to validate? That the logged data is correct, not that the TeeOutputStream's PrintStream output is correct.

> 
> Jacob
> 

Regards,


-- 

Jie Kang