aamtests (EdgeDriver): Compute correct pid & Enable running IA2 tests - #60733
Conversation
spectranaut
left a comment
There was a problem hiding this comment.
Thanks so much for looking into and fixing this :)
| return False | ||
|
|
||
| user32.EnumWindows(check_window_name, LPARAM(0)) | ||
| user32.EnumWindows(check_pid, LPARAM(0)) |
There was a problem hiding this comment.
Instead of deleting/replacing check_window_name, can we instead:
| user32.EnumWindows(check_pid, LPARAM(0)) | |
| if pid: | |
| user32.EnumWindows(check_pid, LPARAM(0)) | |
| else: | |
| user32.EnumWindows(check_window_name, LPARAM(0)) |
If someone wants to test a different browser on windows (see all the browsers that WPT supports: https://github.com/web-platform-tests/wpt/tree/master/tools/wptrunner/wptrunner/browsers) -- they might need to find the browser by name, if their webdriver implementation does not return PID. So it would be nice to leave this fall back :)
benbeaudry
left a comment
There was a problem hiding this comment.
lgtm!
One thought: Microsoft Edge does indeed add more context in its window name so if we do a full comparison, it is bound to fail. But we always append "Microsoft Edge" at the end, so a substring search could work too, but could become a false positive if another browser's name includes the tab name and the browser name and the open tab's name contains "Microsoft Edge". All of this to say that relying on the name alone isn't robust, and I much prefer the new approach based on PID. Thank you!
|
@yezhizhen Thanks for putting this together! I actually opened a very similar PR a few days ago (#60811). Looking at your implementation, I noticed a few differences in how we approached this. In hindsight, my approach for a couple of these was a bit over-engineered, but the first point might still be worth considering:
Overall, your changes look great to me! Aside from the supernitty #1, of potentially updating the returned product name to |
|
@Ms2ger, can you merge this one? The Safari failure is unrelated. edit: nevermind, I could merge this one for whatever reason! |
4767daa
into
web-platform-tests:master
pidwas always missing for EdgeDriver. Now we compute it, with same capability as ChromeDrivergoog:processIDdata:,-personal-microsoftedgeinstead. We use a more robust approach, searching bypidjust identified.Testing:
wpt/core-aam/aamtests/role/blockquote.py
Line 27 in fb83e52
cc @spectranaut @xiaochengh