wpt: Have ServoRefTestExecutor inherit from RefTestExecutor
#40327
Conversation
servo-highfive
added
the
S-awaiting-review
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
|
Given that WPT is out of sync, should we first patch upstream with your fixes and then land them back here? I am worried that the next WPT import is going to change a lot of test expectations and then we would need to revert them once more. If we merge this without being able to upstream the changes, I worry WPT import is unable to run until we address that. |
|
I think the export step has failed only because https://github.com/servo/wpt/ is very old. @jdm is it possible to synchronize the downstream servo/wpt repo manually? |
Ah, never mind. Looks like we do use the upstream copy to create the PR. So I guess I would have to raise the PR in upstream anyway. Although, even if we merge this in Servo first, I think that would still just cause the next import job to fail with merge conflicts. |
|
Something we could do here is to simply rebase and land this after the WPT update tomorrow morning, or run extra WPT update in the meantime. |
|
You can rebase after the import, but I expect a large amount of test expectations update. Since all ref test expectations reset. Not sure how feasible that is to land twice. |
Since `ProcessTestExecutor` is only used by Servo and doesn't add much
on top of `ServoExecutor`, merge the former into the latter.
Also make `ServoExecutor` a mixin so that the derived classes can
inherit from upstream `wptrunner`'s classes and avoid the diamond
inheritance of `TestExecutor` base class.
Another issue is that new changes in the
`RefTestImplementation.get_screenshot_list` method upstream conflicts
with the assumptions in Servo's executor:
- The `dpi` argument is used as an `int` although it is a string
- The `viewport_size` argument is treated as a tuple of ints but it is
in fact a string of the format `WxH`. It also treats the viewport
dimensions as being specified in device pixel coordinates while
Servo treats it as logical coordinates and will scale it by `dpi`.
These issues need to discussed with upstream and patched in
`RefTestImplementation`.
Finally, add back `**kwargs` argument to `ServoCrashtestExecutor`'s
constructor to fix servo#40322.
Fixes servo#40288, servo#40322
Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>
mukilan
force-pushed
the
fix-wpt-reftest-issue-git
branch
from
058408f to
0e00187
Compare
|
🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#55812) with upstreamable changes. |
| super().__init__(logger, browser, server_config, | ||
| timeout_multiplier=timeout_multiplier, | ||
| debug_info=debug_info, | ||
| reftest_screenshot=reftest_screenshot) |
There was a problem hiding this comment.
Do we still need to call super().__init__ here since this only extends Object?
There was a problem hiding this comment.
Yep, this is required to forward the __init__ from the subclass to the sibling base class i.e TestExecutor or RefTestExecutor.
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
servo-highfive
added
S-tests-failed
servo-highfive
added
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
|
⛔ Failed to properly merge the upstream pull request (web-platform-tests/wpt#55812). Please address any CI issues and try to merge manually. |
5 participants
Since
ProcessTestExecutoris only used by Servo and doesn't add muchon top of
ServoExecutor, merge the former into the latter.Also make
ServoExecutora mixin so that the derived classes caninherit from upstream
wptrunner's classes and avoid the diamondinheritance of
TestExecutorbase class.Another issue is that new changes in the
RefTestImplementation.get_screenshot_listmethod upstream conflictswith the assumptions in Servo's executor:
dpiargument is used as anintalthough it is a stringviewport_sizeargument is treated as a tuple of ints but it isin fact a string of the format
WxH. It also treats the viewportdimensions as being specified in device pixel coordinates while
Servo treats it as logical coordinates and will scale it by
dpi.These issues need to discussed with upstream and patched in
RefTestImplementation.Finally, add back
**kwargsargument toServoCrashtestExecutor'sconstructor to fix #40322.
Testing: Tested on fork with changes from upstream's base executor.
Fixes #40288
Fixes #40322