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

NEW
Unassigned

Status

defect
P5
normal
3 years ago
2 years ago

People

(Reporter: vakila, Unassigned, Mentored)

Tracking

({pi-marionette-harness-tests})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=py])

+++ 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
Flags: needinfo?(mjzffr)
Depends on: 1291796
No longer depends on: 1295617
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)
Flags: needinfo?(mjzffr)
(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.
Flags: needinfo?(mjzffr)
+1 for separate parameters for fails, crashes.
Flags: needinfo?(mjzffr)
[mass update] Setting priority
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.