1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

[Robert Wood [:rwood]]

Currently startup tests (i.e. ts_paint) report one value per test iteration. In Bug 1369417 we will have ts_paint measure multiple startup events in a single test iteration, and report multiple values. We need to modify talos to be able to accept multiple values per iteration, and report each value as a 'simulated' separate stand-alone test result, not as subtest values.

Comment 1

[Robert Wood [:rwood]]

With this change, when the tspaint_test.html test reports multiple values in this format (will be done in Bug 1369417):

12:19:52     INFO -  PID 8015 | __start_report{"event_1": 1026, "event_2": 1126, "event_3": 1226}__end_report

Then the resulting PERFHERDER_DATA output will be changed to 'fake' having three separate tests, one per each value that was output in the tspaint test:

12:12:44     INFO -  PERFHERDER_DATA: {"framework": {"name": "talos"}, "suites": [{"subtests": [{"replicates": [1245, 1026], "name": "ts_paint", "value": 1026}], "extraOptions": ["e10s"], "name": "ts_paint"}, {"subtests": [{"replicates": [1345, 1126], "name": "ts_paint_first_paint", "value": 1126}], "extraOptions": ["e10s"], "name": "ts_paint_first_paint"}, {"subtests": [{"replicates": [1445, 1226], "name": "ts_paint_user_ready", "value": 1226}], "extraOptions": ["e10s"], "name": "ts_paint_user_ready"}]}

The test names and event labels are set in the 'tstesteventmap' setting in the ts_paint test class in test.py.

Comment 2

[Robert Wood [:rwood]]

Attachment: Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

Review commit: https://reviewboard.mozilla.org/r/151804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151804/

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

https://reviewboard.mozilla.org/r/151804/#review156832

another option is to have all startup tests output event_1: <value>; then we share the same code for parsing and remove some if/else clauses:)

::: testing/talos/talos/config.py:44
(Diff revision 1)
>          tpchrome=True,
>          tpcycles=10,
>          tpmozafterpaint=False,
> +        tsfirstpaint=False,
> +        tsuserready=False,
> +        tssingletestnames=[],

one concern with the tsXXX naming convention is it is likely we will be measuring this in pageloader tests as well.  I would like to consider dropping ts, so we would have firstpaint and userready.

::: testing/talos/talos/results.py:185
(Diff revision 1)
> +            result = { 'runs': {} }
> +            result['index'] = index
> +            result['page'] = 'NULL'
> +            result['runs']['event_1'] = [jsonResult['event_1']]
> +            result['runs']['event_2'] = [jsonResult['event_2']]
> +            result['runs']['event_3'] = [jsonResult['event_3']]

this assumes there are 3 events and only 3 events- possibly that is fine, but these seem a bit too static.  could we do:
result['runs'] = jsonResult

::: testing/talos/talos/run_tests.py:244
(Diff revision 1)
> +                # run the test
> +                multi_value_result = mytest.runTest(browser_config, test)
> +
> +                # parse out the multi-value results, and 'fake it' to appear like separate tests
> +                test_event_map = test.get('tstesteventmap', None)
> +                separate_results_list = convert_to_separate_test_results(multi_value_result, test_event_map)

do we need to check for values here, specifically that test_event_map!=None, likewise multi_value_result != None?

Comment 4

[Robert Wood [:rwood]]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

https://reviewboard.mozilla.org/r/151804/#review156832

I dont think the effort for that would be worth it, we'd only save one if statement, and I don't really want to risk breaking existing test output. For example, tpaint output is:

14:40:13     INFO -  PID 42716 | __start_report472.095|410.80499999999984|364.4249999999993|377.2300000000005|384.5799999999999|366.619999999999|416.8250000000007|398.3649999999998|344.375|398.71500000000015|394.28499999999985|408.90499999999884|345.8299999999981|408.1399999999994|401.01000000000204|371.22000000000116|395.8199999999997|398.8349999999991|399.47499999999854|360.125__end_report__startTimestamp1498243213276__endTimestamp

and that is not valid json, so that would also have to be converted to valid json first, etc.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

that is fine- can you file a bug and maybe in a future push we can tackle that.  I like to simplify our parsing code and logic for easier debugging and testing in the future.

Comment 6

[Robert Wood [:rwood]]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151804/diff/1-2/

Comment 7

[Robert Wood [:rwood]]

(In reply to Joel Maher ( :jmaher) from comment #5)
> that is fine- can you file a bug and maybe in a future push we can tackle
> that.  I like to simplify our parsing code and logic for easier debugging
> and testing in the future.

Ok, done, filed Bug 1375958

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

https://reviewboard.mozilla.org/r/151804/#review157318

thanks for the updated code

::: testing/talos/talos/run_tests.py:245
(Diff revisions 1 - 2)
> +                multi_value_result = None
> +                separate_results_list = []
> +
> +                test_event_map = test.get('testeventmap', None)
> +                if test_event_map is None:
> +                    print("Abort: can't find 'testeventmap' in test.py for %s" % test.get('name'))

will this look proper on treeherder?  Maybe push to try with testeventmap missing?

Comment 9

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acea370a8265fb19fb6e5aad2a7d8f113524d661

Comment 10

[Robert Wood [:rwood]]

> will this look proper on treeherder?  Maybe push to try with testeventmap
> missing?

Good idea,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed053e33bcf5211603b71b446849e379fb152f90

Comment 11

[Robert Wood [:rwood]]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151804/diff/2-3/

Comment 12

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d59617f6e09f43336cf1b28536089c913baf51e5

Comment 13

[Robert Wood [:rwood]]

https://hg.mozilla.org/try/rev/42650b24d91881c861ecc4bebd78951d79e0161b

Comment 14

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a96f0fca9dc9bdc1fb67d36efa41e2d0d95f3aab

Comment 15

[Robert Wood [:rwood]]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151804/diff/3-4/

Comment 16

[Robert Wood [:rwood]]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151804/diff/4-5/

Comment 17

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64007428e7ea55b6d2996ad26964f4a5a9b42629

Comment 18

[Robert Wood [:rwood]]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151804/diff/5-6/

Comment 19

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=435fb543de408217b024e46c505603dcf65cd82f

Comment 20

[Robert Wood [:rwood]]

Comment on attachment 8880459 [details]
Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151804/diff/6-7/

Comment 21

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81e1139a7556f17dc5e26f649305a1ede9a976bf

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1640a9d57fef
Modify talos startup test framework to support receiving multiple values from a single iteration; r=jmaher

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1640a9d57fef