Tests look great, thanks.

src/python/pants/java/nailgun_client.py (Diff revision 1)

Could this go deeper? into def iter_chunks perhaps?

tests/python/pants_test/java/test_nailgun_client.py (Diff revision 1)

Show all issues

this strikes me as odd - IIUC you redirect sys.stdout to a StringIO then print() (implicitly to the StringIO referenced by sys.stdout) and then retrieve the value?
but as near as I can tell, the whole point here is just to test the default encoding type of print()?
if so, could you not achieve the same thing with:
out_s = StringIO()
print(self.content, file=out_s)
self.stdout_content = out_s.getvalue().strip()

or even simpler by mocking the behavior of print() sans files:
self.stdout_content = self.content.encode(sys.stdout.encoding)


(but this may all be moot after the comment below)

tests/python/pants_test/java/test_nailgun_client.py (Diff revision 1)

Show all issues

seems to me like the scope of these tests should probably continue to test against the raw byte stream as written by write_chunk() (e.g. the wire format) vs stashing and comparing against a unicode conversion this early?
then, as Stu said - you could move the chunk payload encoding to 'utf-8' deeper into NailgunProtocol.iter_chunks and then test the unicode reads using that directly - e.g. in the context the client would read and encode from the byte stream to utf-8 on the read/client-side of the protocol.

tests/python/pants_test/java/test_nailgun_client.py (Diff revision 1)

Show all issues

happy to move the testing out to different call stack in the code but not sure I clearly understand this. 
write_chunk() and iter/read_chunk() as I see are doing the right thing. write_chunk() encodes unicode to bytes before writing it out and iter/read_chunk() decodes bytes to unicode after reading. 
it makes sense to encode in bytes before stdout(..) since stdout of a unicode string causes an implicit encoding - output is always bytes. 
so maybe I am missing something here -- why would we encode again in iter/read(..)?

1. 

   Kris Wilson May 31, 2016, 5:54 p.m.

   well, afaict the reason you have to do a payload.encode('utf-8') in nailgun_client.py here is because of the implicit payload.decode('utf-8') that happens in https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/nailgun_protocol.py#L193
   so the idea here I think - is instead of the payload.encode('utf-8') in nailgun_client.py, you'd do something along the lines of making NailgunProtocol.read_chunk+iter_chunks take parameters that would control this (likely defaulting to decoding to 'utf-8') - but with an option to not do that as used by iter_chunks.
   then in nailgun_client.py you could just do something like:
   for chunk_type, payload in self.iter_chunks(self._sock, decoding=None):
     self._stdout.write(payload)
     ...
     ...'{}'.format(payload.decode('utf-8'))...
     ...
   
   which would mean the change and its tests would move up into nailgun_protocol.py and test_nailgun_procol.py - where all the tests are modeled as a connected pair of sockets that test both sides of the protocol.
   in there, I'm imagining you'd whip up a test to write a properly encoded payload, read it on the other end of the socket while exercising the new options, etc.

2. 

   Ity Kaul May 31, 2016, 6:23 p.m.

   
   
   
   well, afaict the reason you have to do a payload.encode('utf-8') in nailgun_client.py here is because of the implicit payload.decode('utf-8') >that happens in https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/nailgun_protocol.py#L193
   
   afaics, that payload.decode('utf-8') is in the right place. and yes the effect of that is that I get a unicode string in https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/nailgun_client.py#L40 which to me is the right behavior.
   regardless of who the client is, calling a read(..) or iter(..) should only return unicode strings not bytes. I think the right approach is that we should always decode after read because we want unicode strings to be passed around and not bytes -- which is what is happening currently.  let me know if you disagree.

3. 

   Kris Wilson May 31, 2016, 6:37 p.m.

   I definitely agree that payload.decode('utf-8') should be the /default/ case - but in the case of NailgunClient it seems like it'd be more efficient to simply avoid the initial decoding to begin with if we're just immediately calling encode() again, no? afaict, this dual decode/encode will effectively create two copies of every string the NailgunClient processes - one bytes and one unicode. this could add up to 2x additional memory allocation for nailgun/pailgun runs with a bunch of output.

4. 

   Ity Kaul May 31, 2016, 7:30 p.m.

   ok agreed, between clear contract and memory efficiency, efficiency always wins -- it wasnt clear that you were suggesting the removal of decode(..) for memory footprint. adding the new code path then.