This prevents us from running tests on non-rooted devices.
Created attachment 161233 [details] Patch
Comment on attachment 161233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161233&action=review Thank you Philippe! A few more comments. > Tools/ChangeLog:3 > + Support LayoutTests on non-rooted devices for Chromium Android. This should be equal to the bug's title. Maybe this would work? Get rid of device/host clock synchronization for Chromium Android. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:44 > + os.path.join(os.environ['CHROME_SRC'], 'build', 'android', 'pylib')) Peter: I'm not comfortable introducing this dependency. We're trying to get rid of dependencies on environment variable (like CHROME_SRC), and since we need adb commands in order parts of WebKit as well, I'd prefer to have a smaller but similar runner on the WebKit side. This is out of scope, but is there any way we can work around needing this right now? Philippe: I can avoid relying on $CHROME_SRC and use a relative path assuming that this file won't be moved. Regarding the use of android_commands, AndroidCommands.PushIfNeeded() provides a way to sync data files with the device by using a custom implementation of md5sum (written in C++ and depending on Chromium's base/). Although we could have our own Python layer for that in WebKit, we can hardly do the same for md5sum (I don't think you want to duplicate it either). So it seems that we would have to use Chromium's md5sum (being submitted under tools/android/md5sum) anyway. Since we own that file, which is Chromium-specific, I think that preventing us from depending on Chromium might be an unnecessary constraint. I agree that we could introduce at least a thin layer in WebKit, calling Chromium's python, rather than calling it directly though. But I'm sure that I don't see the full picture since this is my first WebKit contribution. What do you think? Is this dependency to Chromium the only one? Peter: The relative path would be different depending on whether layout tests are being run in a Chromium or WebKit environment (from WebKit's root: ../../build/android/pylib/ for Chromium versus Source/WebKit/chromium/build/android/pylib/ for WebKit). I don't think anything else in the layout tests runners depend on Chromium code right now, which also is the reason we have our own adb interface. Since we only need a subset of functionality here, and we need it for running other targets as well, eventually we'll probably end up with a script of our own. Going back to this case: CHROME_SRC will not be available when running layout tests in a WebKit environment, which is my worry. If we can add the md5sum target as a dependency of DumpRenderTree, will that allow you to implement a similar PushIfNeeded method in the ChromiumAndroidDriver? > LayoutTests/http/tests/cache/resources/current-time.cgi:5 > +print time() nit: statements end with a semi-colon. > LayoutTests/http/tests/cache/resources/subresource-test.js:28 > +var serverClientTimeDelta = getServerDate().getTime() - new Date().getTime(); Thanks! Very minor nit for readability: maybe define this property after the getServerDate() function?
Note that you can use self.path_from_chromium_base() in the python code instead of relying on CHROME_SRC. That will work reliably in both a webkit-in-chromium or chromium-in-webkit checkout.
(In reply to comment #3) > Note that you can use self.path_from_chromium_base() in the python code instead of relying on CHROME_SRC. That will work reliably in both a webkit-in-chromium or chromium-in-webkit checkout. Nice. Going back to your comment Peter, can I use what Dirk suggested to include the android_commands module and use AndroidCommands.PushIfNeeded() or do we still really want to avoid this dependency (which would make this patch much larger)? My understanding is that if we have a way to fetch chromium's path, it might not be totally alien to depend on some code coming from it.
(In reply to comment #4) > (In reply to comment #3) > > Note that you can use self.path_from_chromium_base() in the python code instead of relying on CHROME_SRC. That will work reliably in both a webkit-in-chromium or chromium-in-webkit checkout. > > Nice. Going back to your comment Peter, can I use what Dirk suggested to include the android_commands module and use AndroidCommands.PushIfNeeded() or do we still really want to avoid this dependency (which would make this patch much larger)? My understanding is that if we have a way to fetch chromium's path, it might not be totally alien to depend on some code coming from it. There are a number of similar dependencies WebKit has on Chromium already. The implication of starting to rely on test-runner scripts is that refactoring / "API" changes would need the new method to be introduced on the Chromium side, then usage of the old versions removed within WebKit, before the old code can be removed. A cross-project three-way patch is significantly more painful than a single patch. Given the amount of random build breakages due to Chromium infrastructure changes recently, we should try to minimize the number of dependencies until the code stabilizes. While android_commands.py probably will be more stable, our branch still has a 165 line diff and it's not unthinkable more refactoring will happen too. As discussed on chat, adding a dependency to md5sum in DumpRenderTree's gyp configuration is perfectly fine. It'll introduce some additional code for now, but we'll be able to clean that up later again.
Created attachment 161674 [details] Patch
Comment on attachment 161674 [details] Patch Attachment 161674 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13725122
Comment on attachment 161674 [details] Patch Attachment 161674 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13728063
FYI, the try bots fail because this CL depends on https://chromiumcodereview.appspot.com/10867008/ which is not submitted yet.
Created attachment 161680 [details] Patch
I uploaded a new patch. Is that the right way to iterate on a CL? I'm still new to the WebKit contribution process :) Quick question: does ChromiumAndroidDriver need to be thread safe? I added a static variable which is not guarded by any lock at the moment. I supposed that the class was accessed from a single thread. Is that correct?
Comment on attachment 161680 [details] Patch Attachment 161680 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13728069
Comment on attachment 161680 [details] Patch Attachment 161680 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13721160
Created attachment 162197 [details] Patch
I've just re-uploaded the exact same patch now that the other CL on the chromium side has landed. This will re-trigger the try bots which will be green hopefully this time. This is also a way to be back to the top of your inbox :)
Created attachment 162220 [details] Patch
I uploaded a new patch since the old one didn't apply correctly anymore to the current master.
Comment on attachment 162220 [details] Patch Attachment 162220 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13753767
Ah I forgot that we need to roll a new revision of Chromium too. I will re-trigger the try bots by uploading a new patch once we did that.
Comment on attachment 162220 [details] Patch Attachment 162220 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13744971
Created attachment 163098 [details] Patch
Comment on attachment 163098 [details] Patch Attachment 163098 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13804539
Comment on attachment 163098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review A few minor nits. Looks fine overall, except for the build failure (I commented on that later on). > Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:128 > + '<(chromium_src_dir)/tools/android/md5sum/md5sum.gyp:md5sum', This needs to be specific to Android, i.e. in a conditional block. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328 > + if not worker_number in self._data_already_pushed_for_worker: The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:385 > + stdout=subprocess.PIPE).stdout) nit: It's fine to leave it like this (since it may become unreadable otherwise), but WebKit has no limit on line lengths. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:677 > + if last_char in ['#', '$']: nit: could use a tuple here (which is preferred for lists in gyp).
Created attachment 163105 [details] Patch
Comment on attachment 163098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review >> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:128 >> + '<(chromium_src_dir)/tools/android/md5sum/md5sum.gyp:md5sum', > > This needs to be specific to Android, i.e. in a conditional block. Indeed. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328 >> + if not worker_number in self._data_already_pushed_for_worker: > > The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point? I'm still quite new to Python. I wanted to make it a static member. Isn't that the case? I read somewhere that member variables declared in the non-method scope are static.
(In reply to comment #25) > (From update of attachment 163098 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review > > >> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:128 > >> + '<(chromium_src_dir)/tools/android/md5sum/md5sum.gyp:md5sum', > > > > This needs to be specific to Android, i.e. in a conditional block. > > Indeed. > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328 > >> + if not worker_number in self._data_already_pushed_for_worker: > > > > The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point? > > I'm still quite new to Python. I wanted to make it a static member. Isn't that the case? I read somewhere that member variables declared in the non-method scope are static. Ah, I see. You probably should be using "ChromiumAndroidDriver._data_already_pushed_for_worker" to access the variable, then. The rest LGTM.
Created attachment 163125 [details] Patch
Comment on attachment 163098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163098&action=review >>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:328 >>> + if not worker_number in self._data_already_pushed_for_worker: >> >> The _data_already_pushed_for_worker variable is a member of this class, and since this is the __init__ method, won't it always be empty at this point? > > I'm still quite new to Python. I wanted to make it a static member. Isn't that the case? I read somewhere that member variables declared in the non-method scope are static. I have updated this part to refer to the static variable through 'ChromiumAndroidDriver' rather than 'self' which doesn't make sense for a non-instance member.
Thanks! Adam, could you do a formal review please?
Comment on attachment 163125 [details] Patch Attachment 163125 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13796915 New failing tests: http/tests/cache/subresource-expiration-2.html http/tests/cache/subresource-expiration-1.html
I'm looking at the test failures.
I believe the failure is due to the missing execution permission on current-time.cgi. I will re-upload a patch right now.
Created attachment 163137 [details] Patch
Comment on attachment 163137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163137&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410 > + 'Please pass --%s.', configuration, most_recent_binary, most_recent_binary.lower()) As an aside, this warning has always kinda annoyed me since there's no way to make it go away; if you're trying to run both Release and Debug, one will always be newer than the other (unless you hack the mtime, of course). I wonder how much this warning actually helps people. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:385 > + stdout=subprocess.PIPE).stdout) You can use self._port.host.executive.popen() here and below - that's slightly preferred to calling subprocess.Popen() directly.
Created attachment 163313 [details] Patch
Comment on attachment 163137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163137&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410 >> + 'Please pass --%s.', configuration, most_recent_binary, most_recent_binary.lower()) > > As an aside, this warning has always kinda annoyed me since there's no way to make it go away; if you're trying to run both Release and Debug, one will always be newer than the other (unless you hack the mtime, of course). I wonder how much this warning actually helps people. This kind of code is duplicated across multiple places in the Chromium Android Python codebase. We clearly miss a way to figure out the build type in a single line regardless of the language (Python or Shell). I think we have a bug tracking that. If I remember correctly we plan to make GYP output the build type to a file in out/. This code can be updated once we landed that change. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:385 >> + stdout=subprocess.PIPE).stdout) > > You can use self._port.host.executive.popen() here and below - that's slightly preferred to calling subprocess.Popen() directly. Done.
Comment on attachment 163313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163313&action=review Thanks Philippe! > Tools/ChangeLog:5 > + This needs a "Reviewed by" line. As Dirk already reviewed it, it's fine to add "Reviewed by Dirk Pranke". > LayoutTests/ChangeLog:5 > + dito.
Created attachment 163341 [details] Patch
Comment on attachment 163313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163313&action=review >> Tools/ChangeLog:5 >> + > > This needs a "Reviewed by" line. As Dirk already reviewed it, it's fine to add "Reviewed by Dirk Pranke". Done. >> LayoutTests/ChangeLog:5 >> + > > dito. Done.
Created attachment 163345 [details] Patch for landing
Comment on attachment 163345 [details] Patch for landing Putting on the queue. I only touched _setup_md5sum() to make sure it works both on Chromium and on WebKit, of course with Philippe's consent.
Comment on attachment 163345 [details] Patch for landing Clearing flags on attachment: 163345 Committed r128181: <http://trac.webkit.org/changeset/128181>
All reviewed patches have been landed. Closing bug.
This broke webkitpy-test, at least on mac: http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/38183/steps/webkitpy-test/logs/stdio [462/1616] webkitpy.layout_tests.port.chromium_android_unittest.ChromiumAndroidDriverTest.test_cmd_line erred: Traceback (most recent call last): File "/mnt/data/b/WebKit-BuildSlave/chromium-linux-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_android_unittest.py", line 175, in setUp self.driver = chromium_android.ChromiumAndroidDriver(self.port, worker_number=0, pixel_tests=True) File "/mnt/data/b/WebKit-BuildSlave/chromium-linux-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 327, in __init__ self._setup_md5sum() File "/mnt/data/b/WebKit-BuildSlave/chromium-linux-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 340, in _setup_md5sum assert os.path.exists(self._md5sum_path) AssertionError
Yeah it did. Sorry for the inconvenience. The fix has just landed: https://bugs.webkit.org/show_bug.cgi?id=96393.
The Qt bot is happy again: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/26726/steps/webkitpy-test/logs/stdio Closing.
(In reply to comment #36) > (From update of attachment 163137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163137&action=review > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410 > >> + 'Please pass --%s.', configuration, most_recent_binary, most_recent_binary.lower()) > > > > As an aside, this warning has always kinda annoyed me since there's no way to make it go away; if you're trying to run both Release and Debug, one will always be newer than the other (unless you hack the mtime, of course). I wonder how much this warning actually helps people. > > This kind of code is duplicated across multiple places in the Chromium Android Python codebase. We clearly miss a way to figure out the build type in a single line regardless of the language (Python or Shell). I think we have a bug tracking that. If I remember correctly we plan to make GYP output the build type to a file in out/. This code can be updated once we landed that change. > You lost me here. I think you're talking about something different from what I'm talking about; are you talking about picking the "correct" configuration by default so that you don't have to specify either --release or --debug to get the one you want? You can use set-webkit-configuration to specify that. I'm talking about what happens when you have *both* release and debug configurations and are alternating between them - one of them will *always* produce this error.