Integrate with new results receiver #566
Conversation
A new report receiver has been implemented. It will be integrated with this project in a future commit. Both systems will be integrated on a temporary basis. Clearly designate the code for the deprecated receiver in order to limit confusion during this transitional phase.
There was a problem hiding this comment.
Thanks, MIke! Looks like we will be able to remove quite a lot of code later, which is awesome! Git doesn't have great rename detection. You might want to postpone the "_legacy" renaming to a later PR for a cleaner history, but it's up to you.
I just have one concern regarding Sauce Labs.
src/scripts/upload-wpt-results.py
Outdated
| assert isinstance(data['run_info'], object) | ||
| for name in (u'product', 'browser_version', 'os', 'os_version'): | ||
| if name not in report['run_info']: | ||
| report['run_info'][name] = platform_info[name] |
There was a problem hiding this comment.
This might not be correct.
I suspect Sauce runs do have 'product' (or maybe even other fields), but the wrong values (e.g. product=sauce, os=[client os]). Can you get a sample wptreport from a Sauce run? (Doesn't need to be the consolidated version; each shard should also have run_info.)
There was a problem hiding this comment.
You're right. The metadata is present and incorrect. Here's the report generated for a test run in Apple Safari via Sauce Labs:
{
"results": [
{
"message": "/infrastructure/assumptions/ahem.html d4a5ec8885c67c8491d08fa7b04268aba6a19480\n/infrastructure/assumptions/ahem-ref.html ba37c5363f2183149d7267dec4446634846b4721\nTesting d4a5ec8885c67c8491d08fa7b04268aba6a19480 == ba37c5363f2183149d7267dec4446634846b4721",
"status": "FAIL",
"subtests": [],
"test": "/infrastructure/assumptions/ahem.html"
}
],
"run_info": {
"bits": 64,
"debug": false,
"has_sandbox": true,
"linux_distro": "Ubuntu",
"os": "linux",
"os_version": "16.04",
"processor": "x86_64",
"product": "sauce",
"revision": "8c8238ca96f913c1b90f838def7f9cf0253c0113",
"version": "Ubuntu 16.04"
},
"time_end": 1527784288550,
"time_start": 1527784271236
}You've already offered to change this in WPT, but I'd be glad to do it as
well.
Know that the build process is already aware of whether this report comes from
a Sauce-Labs-mediated browser. That means we have the information necessary to
address this without an upstream change. We should improve the WPT CLI
regardless, but that might take some time. If there's urgency here, I could
get this working by the end of the day.
Let me know how you'd like to proceed
There was a problem hiding this comment.
Forgot the link for "You've already offered to change this in WPT": web-platform-tests/wpt.fyi#208 (comment)
There was a problem hiding this comment.
@jugglinmike let's override the metadata for all Sauce runs and Sauce runs only (instead of filling in missing fields, which would also hide issues in Firefox/Chrome metadata that are supposed to be correct).
|
@Hexcles The latest commit introduces an option named |
src/scripts/upload-wpt-results.py
Outdated
| report['run_info'][name] = platform_info[name] | ||
| if platform_override is not None: | ||
| for name in platform_override: | ||
| report['run_info'][name] = platform_override[name] |
There was a problem hiding this comment.
Nit: you could drop is not None. Empty dict/lists are safe to be skipped, too.
And perhaps simply report['run_info'].update(platform_override)
|
Thanks for the review, @Hexcles! It's good to get a reality check, even if it's only cursory. And don't sell yourself short--you found a critical mistake. Anyway, I had hoped to deploy this today, but due to an earlier interruption in the collection process, we're a bit behind schedule for Edge results. Deploying this change will invalidate that collection, so I'd rather defer. Instead of deploying new code on a Friday, I'd like to target Monday morning--will that work for you? |
|
Thanks! Sure, Monday SGTM. Deploying on Friday is the recipe to kill an otherwise great weekend :) |
@rwaldron @Hexcles GitHub doesn't recognize the renames here, but I did this in two commits, so the change set should be easier to digest by viewing those individually.
The new CLI and code uses the property names from the WPT CLI's report. This is a little inconsistent with the "legacy" upload script, but I figured it would be more consistent in the long run.
This is intended to resolve gh-565