Bug 171881 – webkitpy: Run sample/spindump on iOS devices

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

We should enable running both sample and spindump on iOS devices.

[Radar WebKit Bug Importer]

<rdar://problem/32084602>

[Jonathan Bedard]

Created attachment 309542 [details]
Patch

[Jonathan Bedard]

Created attachment 312191 [details]
Patch

[Aakash Jain]

Comment on attachment 312191 [details]
Patch

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

looks fine to me.

> Tools/ChangeLog:3
> +        webkitpy: Preform sample/spindump on iOS devices

"Preform" seems like a typo.

[Daniel Bates]

Comment on attachment 312191 [details]
Patch

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

> Tools/Scripts/webkitpy/port/darwin.py:155
> +    def _need_sudo_for_root():

Can we make use of PlatformInfo to avoid the need to add such a method?

> Tools/Scripts/webkitpy/port/darwin.py:181
> +                                                  DarwinPort.sample_file_path(host, name, pid, self.results_directory()))

This change seems like an unrelated bug fix. In general, a bug/patch should fix one issue. If we have unit tests for this code then it would be good to add a test for this change. If it is not possible to write a test for this change and you feel it is excessive to file a new bug for this change then please add a remark to the ChangeLog for this change.

> Tools/Scripts/webkitpy/port/darwin.py:186
> +                                              DarwinPort.spindump_file_path(host, name, pid, self.results_directory()))

Ditto.

[Jonathan Bedard]

Comment on attachment 312191 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/darwin.py:155
>> +    def _need_sudo_for_root():
> 
> Can we make use of PlatformInfo to avoid the need to add such a method?

Yes, we could directly check if the platform is iOS.

>> Tools/Scripts/webkitpy/port/darwin.py:181
>> +                                                  DarwinPort.sample_file_path(host, name, pid, self.results_directory()))
> 
> This change seems like an unrelated bug fix. In general, a bug/patch should fix one issue. If we have unit tests for this code then it would be good to add a test for this change. If it is not possible to write a test for this change and you feel it is excessive to file a new bug for this change then please add a remark to the ChangeLog for this change.

Actually, this is a change which shouldn't be here at all.  The previous code is correct.

[Jonathan Bedard]

Created attachment 312214 [details]
Patch

[Daniel Bates]

Comment on attachment 312214 [details]
Patch

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

> Tools/Scripts/webkitpy/port/darwin.py:157
> +        command_begin = ['/usr/bin/sudo', '-n'] if self.host.platform.is_ios() else []

We should only call sudo if the platform is Mac.

On another note, can we come up with a better name? command_prefix?

> Tools/Scripts/webkitpy/port/darwin_testcase.py:108
> +        command_begin = ['/usr/bin/sudo', '-n'] if port.host.platform.is_ios() else []

We should not be modifying the expected behavior of a test based on a variable as it negates the value of the test since if the test code is wrong then bugs in the real code can pass through undetected. We should have a separate test that mocks an iOS platform.

> Tools/Scripts/webkitpy/port/darwin_testcase.py:117
> -            if args[0] == '/usr/bin/sudo':
> +            if args[0] == '/usr/bin/sudo' or args[0] == '/usr/sbin/spindump':

Similarly this test methodology is error prone. Hopefully this can be resolved once we do the above.

> Tools/Scripts/webkitpy/port/darwin_testcase.py:132
> +            if args[0] == '/usr/bin/sudo' or args[0] == '/usr/sbin/spindump':

Ditto.

[Jonathan Bedard]

Created attachment 312245 [details]
Patch

[Daniel Bates]

Comment on attachment 312245 [details]
Patch

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

> Tools/Scripts/webkitpy/port/darwin.py:159
> +            str(pid),

Is it necessary to stringify this? I thought Executive will do this for us. If so, then please remove the str().

> Tools/Scripts/webkitpy/port/darwin.py:160
> +            '10',

Similarly, do we need to stringify this? See my above remark.

> Tools/Scripts/webkitpy/port/darwin.py:162
> +            '-file',

Ditto.

> Tools/Scripts/webkitpy/port/darwin.py:165
> +        command = ['/usr/bin/sudo', '-n'] + command if self.host.platform.is_mac() else command

I suggest writing this as a single if statement without an else clause.

> Tools/Scripts/webkitpy/port/darwin.py:171
> +                    str(pid),

Do we need to stringify this? See my remark for line 159.

> Tools/Scripts/webkitpy/port/darwin.py:172
> +                    '10',

Ditto.

> Tools/Scripts/webkitpy/port/darwin.py:173
> +                    '10',

Ditto.

> Tools/Scripts/webkitpy/port/darwin_testcase.py:107
>          port.host.filesystem.files['/__im_tmp/tmp_0_/test-42-spindump.txt'] = 'Spindump file'

We should move this test and the test you modify below as well as any other applicable ones to the test file for the Mac port. We should not be asserting Mac in this test because these tests should be applicable to all Darwin-based platforms.

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:32
> +    os_name = 'ios'

What do we use this class variable for?

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:82
> +            raise ScriptError("MOCK script error")

" => '

> Tools/Scripts/webkitpy/port/ios_simulator_unittest.py:33
> +    os_name = 'mac'

What do we use this class variable for?

[Jonathan Bedard]

Comment on attachment 312245 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/darwin.py:159
>> +            str(pid),
> 
> Is it necessary to stringify this? I thought Executive will do this for us. If so, then please remove the str().

Yes, we do need to stringily this.

>> Tools/Scripts/webkitpy/port/darwin_testcase.py:107
>>          port.host.filesystem.files['/__im_tmp/tmp_0_/test-42-spindump.txt'] = 'Spindump file'
> 
> We should move this test and the test you modify below as well as any other applicable ones to the test file for the Mac port. We should not be asserting Mac in this test because these tests should be applicable to all Darwin-based platforms.

iOS-Simulator ports have an os_name of mac as well.  This test definitely belongs in Darwin.  I assert the os_name because spindump/sample may fail with the wrong os_name, and this failure is not very intuitive.  This assert catches the root-cause of an issue which could break the rest of the test.

>> Tools/Scripts/webkitpy/port/ios_device_unittest.py:32
>> +    os_name = 'ios'
> 
> What do we use this class variable for?

This is used to define platform.os_name.

>> Tools/Scripts/webkitpy/port/ios_simulator_unittest.py:33
>> +    os_name = 'mac'
> 
> What do we use this class variable for?

See above.

[Jonathan Bedard]

Created attachment 312317 [details]
Patch

[Jonathan Bedard]

Fixed the bug which made it necessary to stringify integers.

Added a FixMe regarding the os_name, and removed the assert since after talking with Dan, the hope is that this is a temporary arrangement.

[Daniel Bates]

Comment on attachment 312317 [details]
Patch

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

> Tools/Scripts/webkitpy/port/darwin.py:165
> +        command = ['/usr/bin/sudo', '-n'] + command if self.host.platform.is_mac() else command

I take it you disagreed with by suggestion in comment #10 that it would be more idiomatic to write this line as a single if statement without an else clause:

if self.host.platform.is_mac():
    command = ['/usr/bin/sudo', '-n'] + command

?

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:50
> +

I do not see the value of having this empty line and its presence is inconsistent with the lack of an empty line at the beginning of test_sample_process_exception(). I suggest we remove this empty line.

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:63
> +

Ditto.

[Jonathan Bedard]

Created attachment 312335 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312335 [details]
Patch for landing

Clearing flags on attachment: 312335

Committed r217946: <http://trac.webkit.org/changeset/217946>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.