[webkit-reviews] review granted: [Bug 196323] run-api-tests: Upload test results : [Attachment 366130] Patch

[bugzilla-daemon at webkit.org]

Lucas Forschler <lforschler at apple.com> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 196323: run-api-tests: Upload test results
https://bugs.webkit.org/show_bug.cgi?id=196323

Attachment 366130: Patch

https://bugs.webkit.org/attachment.cgi?id=366130&action=review




--- Comment #6 from Lucas Forschler <lforschler at apple.com> ---
Comment on attachment 366130
  --> https://bugs.webkit.org/attachment.cgi?id=366130
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366130&action=review

>>> Tools/Scripts/webkitpy/api_tests/manager.py:214
>>> +		 self._stream.writeln('')
>> 
>> is there any reason to have two statements here instead of:
>> self._stream.writeln('Test suite failed\n') ?
> 
> Because of this:
> 
> class MeteredStream(object):
>     ...
>     @staticmethod
>     def _ensure_newline(txt):
>	  return txt if txt.endswith('\n') else txt + '\n'
> 
>     ....
>     def writeln(self, txt, now=None, pid=None):
>	  self.write(self._ensure_newline(txt), now, pid)

well... that's interesting...thanks!

>>> Tools/Scripts/webkitpy/port/base.py:1666
>>> +		 scm = SCMDetector(self._filesystem,
self._executive).detect_scm_system(path)
>> 
>> how many times are we going to run through this loop? This seems like it
could be an expensive operation.
> 
> Hopefully not many! Yes, this could be an expensive operation (we maybe
should memoize this, actually), this loop will be run for as many repositories
appleadditions associates with the test run. We will usually run through this
loop 1 or 2 times with the current code.

if we only expect 1 or 2 times, that is fine. in the future we may want to
consider memorization..