[Webkit-unassigned] [Bug 99588] Use forwarder2 in Chrome for Android layout tests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99588





--- Comment #44 from Philippe Liard <pliard at chromium.org>  2012-12-18 04:53:23 PST ---
(From update of attachment 179740)
View in context: https://bugs.webkit.org/attachment.cgi?id=179740&action=review

>> Tools/ChangeLog:40
>> +        (ChromiumAndroidPort.path_to_device_forwarder):
> 
> Please regenerate the ChangeLog.  You can run prepare-ChangeLog to do that.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:39
>> +from webkitpy.common.system.filesystem import FileSystem
> 
> Including this is rarely correct. :)

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:498
>> +                raise AssertionError('Could not push md5sum to the device')
> 
> Seems these paths should be members of this class, if this information is going to be used here?  Or passed in from teh chromium port?  I had trouble with this layering in my attempt at this too.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:501
>> +        return self._adb_command
> 
> WebKit style doesn't normally use get_ for getters.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:529
>> +        assert os.path.exists(host_file)
> 
> Please try to use filesystem.exists instead of os.path.exists so you can mock/unittest your funtions more easily.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:547
>> +class Forwarder:
> 
> All classes should inherit from object in modern python. :)  (At least that's my understanding.)

Done. I'm still pretty new to Python :)

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:570
>> +    def kill_host_daemon(command_executor, host_path_builder):
> 
> Do you reall want these static?  I find @staticmethod to be deceptive in python.  Intially I think "oh, this doesn't need any members" so I make something static... and then I realize it's not easily mockable and cry.  Even @classmethods are more mockable than statics. :)

I'm concerned about the maintenance cost. I would like to keep this consistent with the Forwarder class in Chromium at least from the design perspective if that's ok with you.

We can still improve things later on in a follow up when we also start really benefiting from mocking. It seems to me that this is a bit premature for now since this is not unit tested yet.

To be honest I also would like to get this in soon so that I don't have to deal with nasty rebases anymore :)

Thanks for sharing your experience anyway. This will be useful later on.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:624
>> +        for line in FileSystem().open_text_file_for_reading('/proc/net/tcp').xreadlines():
> 
> Bad.  This makes this method unmockable.  Are you sure you can't pass in a filesystem or Host?

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:651
>> +    @staticmethod
> 
> Generally I've found a single shared instance works out better than a bunch of static methods.  You could split these off into a singleton if that would work better?

See my reply to your comment line 570.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:774
>> +        install_result = self._android_commands.run_adb_command(['install', drt_host_path])
> 
> In my version of this I named this DeviceConnection with the idea that we might some day make it generic not just for android.  That may have been over-design on my part.  But I do think the idea of teaching a "hosted" driver how to talk to the target is generic and not necessarily specific to ADB or android.

I agree with you although I think this is a bit premature for now :)

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:914
>> +        forwarder = Forwarder(self._port._executive.run_command, self._port._build_path, self._android_commands)
> 
> Why not just pass it an executive instead of a run_command method pointer?

I'm injecting only what Forwarder actually needs. It doesn't need to know about executive and this should also make mocking easier since you seem to like it (I can see why) :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.