Closed Bug 1375073 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Talos, enhancement)

Product:
Testing
Component:
Talos
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: rwood, Assigned: rwood)

References

Details

(Whiteboard: [PI:June])

Attachments

(1 file)

Bug 1375073 - Modify talos startup test framework to support receiving multiple values from a single iteration;
59 bytes, text/x-review-board-request
jmaher
: review+
Details 
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.
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 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?
Attachment #8880459 - Flags: review?(jmaher) → review-
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.
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.
(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 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?
Attachment #8880459 - Flags: review?(jmaher) → review+
> will this look proper on treeherder?  Maybe push to try with testeventmap
> missing?

Good idea,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed053e33bcf5211603b71b446849e379fb152f90
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1640a9d57fef
Modify talos startup test framework to support receiving multiple values from a single iteration; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/1640a9d57fef
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: