Include manifest information in test harness error summaries

RESOLVED FIXED in Firefox 54

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(6 attachments)

When treeherder consumes the errorsummary.txt it will need to be able to map which tests belonged to which manifest. It should be relatively easy to get the test harnesses and mozlog to dump this information directly into the errorsummary.

In fact, we should probably combine the manifests.list artifact with the errorsummary.txt and rename it just 'summary.json' (or something like that).

We should definitely discuss the ideal format, but for a first pass I'm thinking something like:

{
  foo/bar/manifest1.ini: ['test_foo failure1 message', 'test_bar failure2 message'],
  foo/baz/manifest2.ini: [],
  ...
}

This way, treeherder can grab the full set of manifests the job ran by inspecting the keys. It can also grab all failures by inspecting values. An empty list would mean all tests in that manifest passed, so it can use len(manifests[key]) for an easy pass/fail check.

We can definitely include additional information in this file or change the format around if it will be useful.
Actually the above data format won't work because if the test suite times out or gets killed, then we'd lose the stored "failures_by_manifest" dict (which needs to be stored and dumped at the end of the test run). I was then thinking we could add a "shutdown" action which would be logged by mozlog itself (e.g when the logger object is garbage collected).. but there's still the chance of the process being SIGKILL'ed.

So I think we'll need to use something similar to the current errorsummary format, and simply stuff a 'manifest' key in there if applicable.
Sorry for all the reviews! But just think how much worse it would be if they were all squashed ;)

As you'll probably notice I dropped the voluptuous idea as it ended up being more invasive than I was hoping. I think ideally we'd still do that at some point, but it's not really worth the effort it would have took.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment on attachment 8840193 [details]
Bug 1340551 - [mozlog] Introduce concept of ContainerType in logtypes and allow nested containers,

https://reviewboard.mozilla.org/r/114680/#review116390

Just trivial fixups, but r+-with-comments seems to be considered bad form.

::: testing/mozbase/mozlog/mozlog/logtypes.py:199
(Diff revision 1)
>  
> -    def convert(self, data):
> -        return dict(data)
> +    def _format_item_type(self, item_type):
> +        superfmt = super(Dict, self)._format_item_type
>  
> +        if isinstance(item_type, dict):
> +            key_type, value_type = item_type.items()[0]

Being able to pass in a dict of length > 1, but ignoring the other values, seems like an issue. Maybe it should raise `ValueError`.

::: testing/mozbase/mozlog/mozlog/structuredlog.py:312
(Diff revision 1)
>                  Unicode("subtest"),
>                  SubStatus("status"),
>                  SubStatus("expected", default="PASS"),
>                  Unicode("message", default=None, optional=True),
>                  Unicode("stack", default=None, optional=True),
> -                Dict("extra", default=None, optional=True))
> +                Dict({Any: Any}, "extra", default=None, optional=True))

This is just the same as `Dict(Any, […])` I think?
Attachment #8840193 - Flags: review?(james) → review-
Comment on attachment 8840194 [details]
Bug 1340551 - [mozlog] Change 'tests' field in suite_start action to a dict of tests keyed by group name,

https://reviewboard.mozilla.org/r/114682/#review116410

::: testing/mozbase/mozlog/mozlog/formatters/machformatter.py:125
(Diff revision 1)
>                                 "assertion_counts": 0,
>                                 "expected": 0,
>                                 "unexpected": defaultdict(int),
>                                 "skipped": 0}
>          self.summary_unexpected = []
> -        return "%i" % len(data["tests"])
> +        tests = chain(*data['tests'].values())

Can't we just write `reduce(lambda s, x: s+x, data["tests"].itervalues(), 0)` here and below to avoid some allocations/overhead.
Attachment #8840194 - Flags: review?(james) → review-
Comment on attachment 8840195 [details]
Bug 1340551 - [mozlog] Add test group (manifest) information to test harness error summaries,

https://reviewboard.mozilla.org/r/114684/#review116428

So… a general question. How are you planning to use this data? I expected that you would add the full list of tests to the summary file, not just add group information when a test fails.

::: testing/mozbase/mozlog/mozlog/formatters/errorsummary.py:29
(Diff revision 1)
>          return "%s\n" % json.dumps(data)
>  
>      def _output_test(self, test, subtest, item):
> +        group = 'default'
> +        for grp, tests in self.tests_by_group.iteritems():
> +            if test in tests:

Are we happy doing a linear search for each element? I don't know if that's better or worse than inverting the hash once.
Attachment #8840195 - Flags: review?(james) → review-
Comment on attachment 8840196 [details]
Bug 1340551 - Log tests by manifest from suite_start in mochitest harness,

https://reviewboard.mozilla.org/r/114686/#review116432
Attachment #8840196 - Flags: review?(james) → review+
Comment on attachment 8840197 [details]
Bug 1340551 - Log tests by manifest from suite_start in marionette-harness,

https://reviewboard.mozilla.org/r/114688/#review116438
Attachment #8840197 - Flags: review?(james) → review+
Comment on attachment 8840198 [details]
Bug 1340551 - Log tests by manifest from suite_start in xpcshell harness,

https://reviewboard.mozilla.org/r/114690/#review116444
Attachment #8840198 - Flags: review?(james) → review+
Comment on attachment 8840195 [details]
Bug 1340551 - [mozlog] Add test group (manifest) information to test harness error summaries,

https://reviewboard.mozilla.org/r/114684/#review116428

Good question.. so my understanding was that treeherder won't care about individual tests unless they fail, so we could add the full test list at the front, but it would be superfluous (at least for the time being).

Let's ask Cam and Ed if they have a preference.

> Are we happy doing a linear search for each element? I don't know if that's better or worse than inverting the hash once.

I don't know either.. I guess inverting the map is likely better
Hey Ed, Cam.. Would it be useful at all to have the full set of tests keyed by group (both ones that passed and failed) at the start of the errorsummary.txt? This would be instead of adding the 'group' key to each failure line, such as in this log:
https://public-artifacts.taskcluster.net/TO2e7MlCQIOO76ZUOTgl9A/0/public/test_info//plain-chunked_errorsummary.log

My understanding is that the new UI going to do anything with passing tests, but maybe it would come in handy somehow?
Flags: needinfo?(emorley)
Flags: needinfo?(cdawson)
*wasn't going to
FWIW I thought you wanted "this chunk contained tests from these groups". But I may be wrong.
Yeah, I think we want "this chunk contained these groups" plus, "this failing test belongs to that group".

Currently the first bit of information can be retrieved by that "manifests.list" file (soon to be renamed "test-groups.list"), and this patch was intended to provide the second piece of information.

But we could definitely merge both these pieces of information into the single "errorsummary.txt" file. Maybe we could rename it 'summary.txt' if we do this, as it would contain data for passing tasks as well.
Also whatever we do, is likely temporary presuming we switch over to a pulse based solution at some point.
I think while we prototype, including that information would be great.  We can always pare it down once we get closer to our final product.
Flags: needinfo?(cdawson)
Depends on: 1342377
Ok, sounds like we should just go this route for now then. I'll also revert the 'manifests.list' artifact as that would no longer be necessary.
Flags: needinfo?(emorley)
On second thought, I'll leave manifests.list in for now as that gives more flexibility on the treeherder end. We can revert whatever we don't end up using later.
Comment on attachment 8840193 [details]
Bug 1340551 - [mozlog] Introduce concept of ContainerType in logtypes and allow nested containers,

https://reviewboard.mozilla.org/r/114680/#review117156
Attachment #8840193 - Flags: review?(james) → review+
Comment on attachment 8840195 [details]
Bug 1340551 - [mozlog] Add test group (manifest) information to test harness error summaries,

https://reviewboard.mozilla.org/r/114684/#review117164
Attachment #8840195 - Flags: review?(james) → review+
Comment on attachment 8840194 [details]
Bug 1340551 - [mozlog] Change 'tests' field in suite_start action to a dict of tests keyed by group name,

https://reviewboard.mozilla.org/r/114682/#review117260
Attachment #8840194 - Flags: review?(james) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f5ee692c956
[mozlog] Introduce concept of ContainerType in logtypes and allow nested containers, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/9faf01462771
[mozlog] Change 'tests' field in suite_start action to a dict of tests keyed by group name, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/1fb190001b3e
[mozlog] Add test group (manifest) information to test harness error summaries, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/b2b8e0f9fe10
Log tests by manifest from suite_start in mochitest harness, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/762d295b35b9
Log tests by manifest from suite_start in marionette-harness, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/3026fb03c97d
Log tests by manifest from suite_start in xpcshell harness, r=jgraham
You need to log in before you can comment on or make changes to this bug.