Summary: | [Chromium-Android] Virtual test suites fail | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||||||
Component: | Tools / Tests | Assignee: | Xianzhu Wang <wangxianzhu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dpranke, kbalazs, ojan, peter, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Android | ||||||||||
OS: | Android | ||||||||||
Attachments: |
|
Description
Xianzhu Wang
2012-07-27 09:38:44 PDT
No, you can't move this into Driver.run_test() because the command line args might change, leading you to need to restart the driver (defeating the point of DriverProxy). However, if you're willing to accept restarts yourself, you can create a ChromiumAndroidDriver that subclasses Driver and override run_test() as needed. The longer-term fix is to support the other command line flags on a per-test basis. Once we get https://bugs.webkit.org/show_bug.cgi?id=92398 to land, we'll have most of the logic in place and then we extend it for the chromium ports (and anyone else that wants to use virtual suites) to support other sets of command line flags. (In reply to comment #1) > No, you can't move this into Driver.run_test() because the command line args might change, leading you to need to restart the driver (defeating the point of DriverProxy). > > However, if you're willing to accept restarts yourself, you can create a ChromiumAndroidDriver that subclasses Driver and override run_test() as needed. > > The longer-term fix is to support the other command line flags on a per-test basis. > > Once we get https://bugs.webkit.org/show_bug.cgi?id=92398 to land, we'll have most of the logic in place and then we extend it for the chromium ports (and anyone else that wants to use virtual suites) to support other sets of command line flags. Thanks for explanations. I think I'd better make a workaround in our downstream first and wait for the changes about per_test_args landed in upstream. Created attachment 156735 [details]
Patch
(In reply to comment #3) > Created an attachment (id=156735) [details] > Patch Do you still need DriverProxy? Ref tests are now supported with a simple driver instance since the harness pass the pixel test option for each test that need it. Dpranke and I are actually planning to remove it: bug 93269. I hope you can handle the special config of virtual test suites by extending the command line flags of tests. (In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=156735) [details] [details] > > Patch > > Do you still need DriverProxy? Ref tests are now supported with a simple driver instance since the harness pass the pixel test option for each test that need it. Dpranke and I are actually planning to remove it: bug 93269. I hope you can handle the special config of virtual test suites by extending the command line flags of tests. The Android port still needs the DriverProxy for the same reason the other Chromium ports do; however, since the Android port still has its own driver, we could just move the retstart-if-command-line-is-different logic directly into the ChromiumAndroidDriver code. As you say, it would be good to update the port to support the other command line args on a per-test basis as well. Comment on attachment 156735 [details]
Patch
Will upload a new patch soon.
Created attachment 156760 [details]
This patch doesn't use DriverProxy
Created attachment 156766 [details]
landing
Comment on attachment 156766 [details] landing Clearing flags on attachment: 156766 Committed r124810: <http://trac.webkit.org/changeset/124810> All reviewed patches have been landed. Closing bug. |