<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Use multiple iOS simulator instead of multiple apps on single simulator for running layout-tests"
href="https://bugs.webkit.org/show_bug.cgi?id=151243#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Use multiple iOS simulator instead of multiple apps on single simulator for running layout-tests"
href="https://bugs.webkit.org/show_bug.cgi?id=151243">bug 151243</a>
from <span class="vcard"><a class="email" href="mailto:dean_johnson@apple.com" title="Dean Johnson <dean_johnson@apple.com>"> <span class="fn">Dean Johnson</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=265510&action=diff" name="attach_265510" title="Updated patch">attachment 265510</a> <a href="attachment.cgi?id=265510&action=edit" title="Updated patch">[details]</a></span>
Updated patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=265510&action=review">https://bugs.webkit.org/attachment.cgi?id=265510&action=review</a>
r=me after the comments, although I can't speak for the non-Python changes.
<span class="quote">>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:82
>> + self.NUM_PROCESSES_PER_SIMULATOR = 100
>
> Existing code uses "num" a lot, but I think that we should do better. First, it's an abbreviation, and second, it's not very clear.
>
> Maybe something along the lines of "PROCESS_COUNT_ESTIMATE_PER_SIMULATOR_INSTANCE"?</span >
I think this naming conventions is fine and accurate.
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:87
> + plimit = int(subprocess.check_output(["launchctl", "limit", "maxproc"]).strip().split()[1])</span >
I think it'd be easier to do:
limit = int(subprocess.check_outpu(["ulimit", "-u"]))
<span class="quote">>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:88
>> + current_num_process = len(subprocess.check_output(["ps", "aux"]).strip().split('\n'))
>
> Same comments about abbreviations. Maybe "system_process_count_limit" or "maxproc"? "current_process_count"?</span >
current_num_process -> current_num_processes
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:90
> + num_sim = (plimit - current_num_process) / self.NUM_PROCESSES_PER_SIMULATOR</span >
Nit: If we want to strictly do integer division we should probably use // instead of /. (This doesn't apply to Python2, but explicit is better than implicit :))
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:91
> + except:</span >
Which exception are you trying to catch? subprocess.CalledProcessError? We should be explicit in what we're catching so we don't run into edge cases of excepting something else unexpectedly.
<span class="quote">>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:93
>> + num_sim = self.MAX_NUM_OF_SIMULATOR
>
> When do we get here? Does it need to be a recoverable error, or can the script just die?</span >
This will cause issues if we don't raise the process limit cap. A quick sampling of our bots shows that around 330 processes are running on them, which means we will hit the default limit (709) spawning any more than 3 simulators.
I think if we fail on shell'ing out to the appropriate commands we should simply default to 1 simulator, but I don't see a case where we should fail to run the commands if we use ps and ulimit.
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:158
> + test_inputs = self._get_test_inputs(test_names, self._options.repeat_each, self._options.iterations)</span >
Do these options default to integer values, or strings?
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:159
> + num_workers = self._runner.get_num_workers(test_inputs, int(self._options.child_processes))</span >
If the above options DO default to integers, we should probably follow the same convention for child_processes.
<span class="quote">>> Tools/Scripts/webkitpy/port/base.py:885
>> + def reset_preferences(self, num_workers=1):
>
> As elsewhere, it would help to elaborate on "num" - is this requested number, maximum number, or something else?</span >
I think num_workers is fine. I'd assume num_workers is the number of workers to use.
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:203
> + device_udid = self.testing_device(i).udid</span >
Why is testing_device a function? Shouldn't it be `testing_devices` as an array?
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:212
> + Simulator.wait_until_device_is_booted(self.testing_device(i).udid)</span >
Ditto on testing_device comment.
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:233
> + except:</span >
Can we explicitly catch an exception here instead of catching all cases?
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:255
> + testing_device = self.testing_device(i)</span >
Ditto to testing_device comment above.
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:256
> + _log.debug('Verifying Simulator' + str(i) + ' with UDID {0}.'.format(testing_device.udid))</span >
This is more idiomatic:
_log.debug('Verifying Simulator{0} with UDID {1}.'.format(i, testing_device.udid))
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:335
> + def testing_device(self, number):</span >
Ah, I see why you are using it as a function now. Still, maybe we should create all the testing_devices and put them in an array to access?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>