-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract Safari version from safaridriver #13467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sort of works but is only half done. Sent out with comments hoping that @jgraham or @jugglinmike might have ideas. For what's left a mystery I will dig in myself :)
tools/wpt/browser.py
Outdated
output = output[14:] | ||
return output | ||
except subprocess.CalledProcessError: | ||
# TODO stderr gets printed :'( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found that call
doesn't suppress stderr and just prints it instead. This isn't great because this is expected to fail on every safaridriver
before STP 67, notable including Safari 12 stable. Should I just change call
to return a stdout,stderr tuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's surprising; it looks like it calls subprocess.checkou_output
with no arguments. I would suggest making it take **kwargs
and passing those down to the caller so you can redirect stderr to /dev/null
or whatever.
tools/wpt/browser.py
Outdated
except subprocess.CalledProcessError: | ||
# TODO stderr gets printed :'( | ||
pass | ||
# TODO: this message doesn't show up anywhere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this, is the logger not wired up correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logger handler gets wired up late, but we should probably fix that.
Needs to be rebased on top of #13642 now. |
3e6d406
to
a35a216
Compare
@foolip could your rebase this and fix the unit tests? |
a35a216
to
75f0e0a
Compare
Alright, this finally works! A snippet from a wpt_report.json produced locally:
(It works when using |
No description provided.