Bug 92515 – [Chromium-Android] Virtual test suites fail

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

From http://trac.webkit.org/changeset/122780 (bug 88542), ChromiumAndroidPort doesn't use DriverProxy but uses ChromiumAndroidDriver directly because we don't want DriverProxy to create two drivers for us. ChromiumAndroidDriver switches between pixel and non-pixel mode by itself.

However, the code to support virtual test suite is in DriverProxy, so now the feature is broken on Chromium-Android.

Dirk, can we move the following code from DriverProxy.run_test() into Driver.run_test()?

        base = self._port.lookup_virtual_test_base(driver_input.test_name)
        if base:
            virtual_driver_input = copy.copy(driver_input)
            virtual_driver_input.test_name = base
            virtual_driver_input.args = self._port.lookup_virtual_test_args(driver_input.test_name)
            return self.run_test(virtual_driver_input)

I tried that but it broke some unit tests. I'm wondering if the change would break some ports. If not, I could fix the unit tests.

[Dirk Pranke]

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.

[Xianzhu Wang]

(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.

[Xianzhu Wang]

Created attachment 156735 [details]
Patch

[Balazs Kelemen]

(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.

[Dirk Pranke]

(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.

[Xianzhu Wang]

Comment on attachment 156735 [details]
Patch

Will upload a new patch soon.

[Xianzhu Wang]

Created attachment 156760 [details]
This patch doesn't use DriverProxy

[Xianzhu Wang]

Created attachment 156766 [details]
landing

[WebKit Review Bot]

Comment on attachment 156766 [details]
landing

Clearing flags on attachment: 156766

Committed r124810: <http://trac.webkit.org/changeset/124810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.