1298900 - [marionette harness tests] Improve readability of tests/fixtures using `num_fails_crashed`

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, defect, P5)

Description

[Anjana Vakil (:vakila)]

+++ This bug was initially created as a clone of Bug #1295617 +++

In some of the Marionette harness unit tests related to the way crashes/failures are handled, the `num_fails_crashed` parameter is used in a non-obvious way. 

The values of this parameter are tuples with two values (e.g. [1]), which are unpacked in the `harness_class` and `runner_class` fixtures ([2]). In one case ([3]), this parameter is declared alongside another parameter, such that the values passed to `pytest.mark.parametrize` are nested tuples. 

In general, it would be helpful to keep parameters limited to a single value, stacking `parametrize` decorators if necessary to combine multiple parameters (e.g. [4]). This would help with code readability, and also make it easier to create human-readable parameter IDs (see Bug 1295617).

We should therefore reexamine the use of `num_fails_crashed` and related parameters to see if there's a more readable way we can rewrite these tests/fixtures.


[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#203

[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#156-190

[3] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#193-196

[4] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#348-349

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Some quick thoughts.

(In reply to Anjana Vakil (:vakila) from comment #0)
> The values of this parameter are tuples with two values (e.g. [1]), which
> are unpacked in the `harness_class` and `runner_class` fixtures ([2]). 

In https://dxr.mozilla.org/mozilla-central/rev/506facea63169a29e04eb140663da1730052db64/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_harness.py#62, a key concern is that we don't want to test all possible combinations. 

>In
> one case ([3]), this parameter is declared alongside another parameter, such
> that the values passed to `pytest.mark.parametrize` are nested tuples. 

In https://dxr.mozilla.org/mozilla-central/rev/506facea63169a29e04eb140663da1730052db64/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_harness.py#54, we need to pair the test data (num crashes + failures) with an expected outcome (the exit code, the 3rd number in that nested tuple)

Comment 2

[Anjana Vakil (:vakila)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #1)
> Some quick thoughts.
> ...
> In
> https://dxr.mozilla.org/mozilla-central/rev/
> 506facea63169a29e04eb140663da1730052db64/testing/marionette/harness/
> marionette/tests/harness_unit/test_marionette_harness.py#62, a key concern
> is that we don't want to test all possible combinations. 

Sure, and this is a good argument for not splitting the parameters into stacked decorators. But wouldn't it still be clearer to have them as separate parameters, e.g. `parametrize('fails,crashes', [(0,0), ...])`, instead of lumped together as `num_fails_crashed`? That way we wouldn't have to unpack the tuple later.

> In
> https://dxr.mozilla.org/mozilla-central/rev/
> 506facea63169a29e04eb140663da1730052db64/testing/marionette/harness/
> marionette/tests/harness_unit/test_marionette_harness.py#54, we need to pair
> the test data (num crashes + failures) with an expected outcome (the exit
> code, the 3rd number in that nested tuple)

Understood. But similar to the above, wouldn't flat tuples a la `parametrize('fails,crashes,exit_code', [(0,0,1), ...])` be more readable than nested tuples?

Otherwise, I'm fine with leaving this as a wontfix.

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

+1 for separate parameters for fails, crashes.

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Links in Comment 0 should have referred to a specific revision. The code has moved around a lot since then, so here are updated permalinks from a recent revision:

[1] https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_harness.py#61

[2] https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_harness.py#14-48

[3] https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_harness.py#51-54

[4] https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py#163-164

Comment 5

[David Burns :automatedtester]

[mass update] Setting priority