multiple reports in same json file #132
Conversation
moz-webgpu-cts/src/main.rs
Outdated
| // Servo has multiple reports in same json file (each in one line) | ||
| // First one is from first run, second one run that runs only unexpected results for filtering |
There was a problem hiding this comment.
praise: I think it's great context to note motivation for doing this.
There was a problem hiding this comment.
suggestion(non-blocking): Maybe it'd be interesting to re-phrase and say that Servo's wptreport.json files essentially a two-record JSONL file?
There was a problem hiding this comment.
suggestion(if-minor): If this behavior is Servo-specific, I think it might be better to gate it behind a matches!(browser, Browser::Servo) check?
There was a problem hiding this comment.
suggestion(if-minor): If this behavior is Servo-specific, I think it might be better to gate it behind a
matches!(browser, Browser::Servo)check?
It is, but it should be compatible with normal wptreports files (from firefox). Actually servo's wptreport.json can also contain only one object (one run, no unexpected results, or no filtering requested).
There was a problem hiding this comment.
@sagudev: I understand that this is backwards-compatible with existing wptreport.json documents, and I agree that it is a nice property. However, I think that silent handling of this superset should still be opt-in on some basis. If there were multiple concatenated JSON values in a vanilla wptreport.json, that would more likely mean that something has gone wrong (though I suppose the structure of each value would be checked 🤔). I'd rather an error or warning be emitted by default, for now.
There was a problem hiding this comment.
It is, but it should be compatible with normal wptreports files (from firefox). Actually servo's wptreport.json can also contain only one object (one run, no unexpected results, or no filtering requested).
I guess I'm confused about this use case's justification, then. This feature seems like it'd only make sense if you could only provide a JSONL document to moz-webgpu-cts, but you're noting that Servo emits a single JSON object per document, too. Is there a reason that you want to feed concatenated JSON objects in a single file to moz-webgpu-cts, instead of providing it multiple file paths?
There was a problem hiding this comment.
RE: justification: I'm guessing that there's an intermediate step in Servo's process that divides a single report into two partitioned JSON objects?
There was a problem hiding this comment.
RE: justification: I'm guessing that there's an intermediate step in Servo's process that divides a single report into two partitioned JSON objects?
Yes, and given that in the future we can also use those results for intermittent, this additional step in servo would only make it harder.
CI runs do filtering, hence you get to objects in same JSON file, but local runs are usually done without filtering (less CLI flags needed) so it only creates one object in JSON file.
There was a problem hiding this comment.
I've added warning if there are any leftover objects in JSON (if not servo) in 905b6b8
ErichDonGubler
force-pushed
the
multiple-in-one
branch
from
282408e to
a404692
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Just to be sure, firefox's wptreport.json is also one line json file, right?