[mozlog] Add manifest to test objects in errorsummary.log

RESOLVED FIXED in Firefox 55

Status

Testing
Mozbase
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: camd, Assigned: ahal)

Tracking

(Blocks: 2 bugs)

Version 3
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

5 months ago
Treeherder will at some point be ingesting and storing the entire list of Manifests and any tests that failed for it.  This data will be used for the Test-Group based UI currently under development.

It would greatly help with data ingestion to have a file with this data in the following format:

{
    "suite": "mochitest",
    "manifests": [
        "manifest-foo.ini": [],  # all tests within manifest passed
        "manifest-bar.ini": [
            "test-abc-1.html": [
                {"status": "fail", ...},
                {"stackwalk_stdout": "...", ...}
            ],
        ],
        ...
    ],
}

or perhaps it would be more like:

{
    "suite": "mochitest",
    "manifests": [
        "manifest-foo.ini": [],  # all tests within manifest passed
        "manifest-bar.ini": [
            {"test": "test-abc-1.html", "status": "fail", ...},
            {"test": "test-abc-2.html", "stackwalk_stdout": "...", ...}
        ],
        ...
    ],
}

This is a structure Ed came up with when I talked to him today (with a few tweaks).
(Reporter)

Comment 1

5 months ago
Hey Andrew, here's the bug we talked about today.  Thanks heaps!  :)
Flags: needinfo?(ahalberstadt)
(Reporter)

Updated

5 months ago
Blocks: 1337488
Requiring a single json structure is bad for streaming. Just a list of tests from suite_start would be OK; a list of failing results is problematic because if the process dies we end up with either entirely missing data or corrupted JSON.
(Assignee)

Comment 3

5 months ago
Hm, true.. Also if we ever wanted to fail fast (i.e turn a job orange as soon as a failure came in over pulse or something), this structure would prevent that.

Cam, is this solution more about reducing code complexity or reducing computation? If it's the former, maybe it would be better to just live with it.. if the latter, maybe we need to rethink using artifacts in the first place :/
Flags: needinfo?(ahalberstadt) → needinfo?(cdawson)

Comment 4

5 months ago
(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Cam, is this solution more about reducing code complexity or reducing computation?

It was a bit of both, but I think we're happy to forgo given the streaming and crashing mid run considerations.

What we do really need however, if possible, is to have the correlation between manifest and failing test to be made in that file, rather than having to cross-reference.

So:

{"manifests": [m1, m2, m3])
{"testname": "foo", "manifest" m1, status: fail}

Differences compared to current:
* The `manifests` key lists all manifests (regardless of pass/fail), but no longer lists every test name (since this bloats the file, and Treeherder won't be ingesting the name of every passing test due to data volume).
* The row for each failure not only lists the test name, but also the manifest from which that test was run (to save Treeherder having to cross-reference).

Does that sounds viable?
(Reporter)

Comment 5

5 months ago
Heh, what Ed said, yeah.  :)  It's not even that much extra work to have them separate.  But the scheme Ed mentioned will be totally fine to work with.  Thanks for chiming in, guys.  :)
Flags: needinfo?(cdawson)
(Assignee)

Comment 6

5 months ago
Great, makes sense! Should be an easy change, I'll have it up shortly.
(Assignee)

Updated

5 months ago
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Summary: Create a manifest-centric Mozbase error summary file → [mozlog] Add manifest to test objects in errorsummary.log
Comment hidden (mozreview-request)
(Assignee)

Comment 8

5 months ago
Still waiting for a failing task in my try push, but here's what a passing errorsummary looks like:
https://public-artifacts.taskcluster.net/feT-gZ7UTdu3VSLDiiu4Sg/0/public/test_info//plain-chunked_errorsummary.log

Failure lines should have a "group" key in them now that contains the manifest.
(Assignee)

Updated

5 months ago
Attachment #8875339 - Flags: review?(james)
(Assignee)

Comment 9

5 months ago
Looks like chrome and a11y test paths have chrome:// in front, which makes them hard to match against the list of tests dumped at the beginning (which are relative paths to topsrcdir). I can't simply munge them in errorsummary.py, as that would break other harnesses where urls are actually desired.

I guess I'll try to get the mochitest harness to log just the test path. Browser and plain mochitests already do this.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

5 months ago
Here's an example of the new format (with failures):
https://public-artifacts.taskcluster.net/amNaNFDLQa-7PJ3qvxJZIA/0/public/test_info//browser-chrome-chunked_errorsummary.log

Comment 13

5 months ago
mozreview-review
Comment on attachment 8875339 [details]
Bug 1370359 - Stop dumping all tests to errorsummary.log; include manifest in error lines,

https://reviewboard.mozilla.org/r/146752/#review151226
Attachment #8875339 - Flags: review?(james) → review+

Comment 14

5 months ago
mozreview-review
Comment on attachment 8875417 [details]
Bug 1370359 - Add ally to list of test path prefixes that mochitest harness normalizes,

https://reviewboard.mozilla.org/r/146862/#review151228
Attachment #8875417 - Flags: review?(james) → review+

Comment 15

5 months ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/189df493159b
Add ally to list of test path prefixes that mochitest harness normalizes, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/09b1aee5ad9f
Stop dumping all tests to errorsummary.log; include manifest in error lines, r=jgraham
(Reporter)

Comment 16

5 months ago
This looks awesome!  Thanks, Andrew!  :)

Comment 17

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/189df493159b
https://hg.mozilla.org/mozilla-central/rev/09b1aee5ad9f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

4 months ago
Blocks: 1347945
You need to log in before you can comment on or make changes to this bug.