scripts: twister: Elevate Status Error #77080
Conversation
LukaszMrugala
added
DNM
zephyrbot
requested review from
gchwier,
golowanow,
hakehuang,
KamilxPaszkiet,
nashif and
PerMac
| logger.error(f'Harness assigned status "{value}"' | ||
| f' without an equivalent in TwisterStatus.' | ||
| f' Assignment was ignored.') | ||
| error_msg = f'Harness assigned status "{value}"' \ |
There was a problem hiding this comment.
What if to implement StatusError more like ConfigurationError:
- with parameters (where/what) was failed instead of long text message.
- with the exception handler in 'runner.py' and logging there.
There was a problem hiding this comment.
I agree with 1..
What would be the correct handling on the runner.py level - setting the status to ERROR, I presume?
There was a problem hiding this comment.
wrong status assignment is Twister internal problem, it should stop on it, isn't it ?
There was a problem hiding this comment.
On one hand yes, it is a Twister problem, but it is, at least in theory, contained within one instance. So I think ERRORing the instance and continuing is more appropriate. In the end, ERROR is made for tests failed for reasons outside of the tests' scope (e.g. Build errors).
There was a problem hiding this comment.
IMO it is not reliable and an extra complexity to continue execution in this case when there is some logic issue at Twister itself.
| @@ -128,7 +128,7 @@ def test_handler_final_handle_actions(mocked_instance): | |||
| handler.suite_name_check = True | |||
|
|
|||
| harness = twisterlib.harness.Test() | |||
| harness.status = mock.Mock() | |||
| harness.status = 'NONE' | |||
There was a problem hiding this comment.
can you please also test it for harness.status = None
LukaszMrugala
force-pushed
the
lmrugalx_status_error_enchancement
branch
from
5669380
to
f27d671
Compare
LukaszMrugala
force-pushed
the
lmrugalx_status_error_enchancement
branch
from
f27d671
to
5e64ef1
Compare
LukaszMrugala
force-pushed
the
lmrugalx_status_error_enchancement
branch
3 times, most recently
from
574fe41
to
30b9cb0
Compare
|
Changes in the rebase, for ease of re-review:
|
Status errors previously logged an error, but didn't fail the running test. This commit changes that and introduces a new StatusAttributeError to use there. One test is modified so it follows proper status form. One test for the new error has been added. Status errors now will properly mark the Instance as ERROR and not run TestCases as SKIP. This necessitated some code layout changes in runner.py Signed-off-by: Lukasz Mrugala <[email protected]>
LukaszMrugala
force-pushed
the
lmrugalx_status_error_enchancement
branch
from
30b9cb0
to
bd0e9e3
Compare
|
This PR's tests should be rerun before merging if #77168 is merged before it. |
Status errors previously logged an error, but didn't fail the running test.
This commit changes that and introduces a new
StatusErrorto use there.Additionally, one test is modified so it follows proper status form.