Log file names generated by gtest-parallel are too long about gtest-parallel HOT 8 CLOSED

Do you think this could be solved alongside #54? The proposed solution there is that we have --output_dir be None by default (and that just eats the passing logs, not spitting them out like 1. proposed here). When the output_dir is None, maybe we can use shorter tmpfile names and clear the temporary directory on exit?

Importantly: We need to save the log files to disk even when output_dir is unset (or go to /dev/null directly). Piping stdin/stdout to Python using the subprocess commands have been slow/buffering/unpredictable and causing hangs on WebRTC's build bots where a command can stall for multiple seconds. This was fixed in commit 34ae4d7.

from gtest-parallel.

.. and I think the output can't go to /dev/null directly, because then we can't get logs from failing processes. But we could definitely have shorter names for the interrim log files, maybe generated by mktemp, (as users don't have to get to them), and we shouldn't have to move the files on completion either (to passed/, failed/ etc.).

from gtest-parallel.

Do you think this could be solved alongside #54?

yes, this one is closely related.

Importantly: We need to save the log files to disk even when output_dir is unset

I don't understand this. What I was thinking about when output_dir is None in Task.run() instead of this code:

    with open(self.log_file, 'w') as log:
      task = subprocess.Popen(self.test_command, stdout=log, stderr=log)

log could be in-memory file stream, say, StringIO. Is this what you tried to warn against when stating "Piping stdin/stdout to Python using the subprocess commands have been slow..."?

from gtest-parallel.

I think it would be fine if log was an in-ram file handle on the OS level, what we had performance issues with was: stdout = subprocess.PIPE, stderr = subprocess.STDOUT in the Popen call. I'm not sure what in the implementation make it so (that makes me wary of python-implemented StringIO etc).

There's a possibly-related post here: https://stackoverflow.com/a/10889177 but in our use we found delays / buffering / whatever that would not only delay the runtime with a constant factor but also see occasional bumps resulting both in timeouts (looking like flaky tests) and runtimes not representative of the underlying test runtime. We should add as little as possible to test runtimes or we're essentially lying. :)

from gtest-parallel.

Perhaps something like tempfile.mkstemp or even tempfile.TemporaryFile could be used. They seem more like wrappers around real OS functions. A big upshot for not using in-memory backing is that I've seen some huge tests that essentially spit out raw video frames to stdout. This would kill having a memory-backed file (and even if it's stupid I don't think we should call it unsupported).

from gtest-parallel.

So, to summarize. If output_dir is None we save to temporary file created with tempfile.mkstemp. If a test fails we dump log file's content to stdout (like it's done right now) and also, in this case, we can safely remove the temporary file (as no one need it in a temporary directory). tempfile.TemporaryFile is not a fit as the file is removed once it is closed.

Does this look reasonable?

from gtest-parallel.

Yep, sounds good. TemporaryFile could work if we keep an open handle to it for reading later, but whatever's easiest tbh.

Yes, once the test has finished running we should remove the file, regardless of if we print the contents before or not. If there are any finally: or destructor things that we could run those could ensure that the file is deleted even if someone Ctrl+C's the commands (but I don't know python well enough to know if that's easily applicable).

If you implement this change I can take care of documentation updates if you'd prefer.

from gtest-parallel.

See pull request #59.

And yes please update the documentation. You'll definitely come up with better wording.

from gtest-parallel.