Fix bug in BaseRestartWorkChain.inspect_process
#4166
Conversation
sphuber
added
the
pr/blocked
Codecov Report
@@ Coverage Diff @@
## develop #4166 +/- ##
===========================================
- Coverage 78.90% 78.89% -0.01%
===========================================
Files 467 467
Lines 34481 34481
===========================================
- Hits 27203 27199 -4
- Misses 7278 7282 +4
Continue to review full report at Codecov.
|
sphuber
force-pushed
the
fix/4165/base-restart-report-exit-code
branch
from
ff0b1e1 to
5d8f1d4
Compare
| # Exit status 400 of the last calculation job will be handled and so should reset the flag | ||
| process.ctx.children.append(generate_calculation_node(exit_status=400)) | ||
| result = process.inspect_process() | ||
|
|
||
| # Even though `handler_a` was followed by `handler_b`, we should retrieve the exit_code from `handler_a` because | ||
| # `handler_b` returned `None` which should not overwrite the last report. | ||
| assert isinstance(result, ExitCode) | ||
| assert result.status == 0 | ||
| assert result.status == 418 | ||
| assert result.message == 'IMATEAPOT' | ||
| assert process.ctx.unhandled_failure is False |
There was a problem hiding this comment.
If we are testing the reset, and although it might be redundant because you already check that a calculation with an exit status != 0 sets off the process.ctx.unhandled_failure, shouldn't it be anyways better to check that the status actually changed and was set back (instead of maybe just always staying False)? (so perhaps assert process.ctx.unhandled_failure is True just before calling result = process.inspect_process())
There was a problem hiding this comment.
I may misunderstand your point, but the unhandled_failure is only changed by the workchain in the inspect_process call. So adding the assert process.ctx.unhandled_failure is True check before the call will fail.
There was a problem hiding this comment.
No, I think you understood correctly. But then maybe the docstring of the test should say "Test ctx.unhandled_failure is set to False in inspect_process when a failed process is handled." or something like this? I mean, this is probably a nitpick, but using "reset" makes me think that at some point the ctx.unhandled_failure is set to True (like it happens in the previous test), which is not the case here from what I am seeing in the code right? Or perhaps I am not understanding what you mean by "reset"...
There was a problem hiding this comment.
Ah I see now, you are right. There is a mistake. The idea of the test is that there is first a failed calculation, which sets unhandled_failure=True followed by another failed calculation, but that one returns a report, so it is not considered unhandled and so should reset the flag. The first calculation shouldn't be 0 then but some non-zero code. Will push a fix.
There was a problem hiding this comment.
Ah, ok, that makes more sense, haaha. This was why I was asking why you needed this previous calculation in the other PR. Good catch (Y)
There was a problem hiding this comment.
Good catch (Y)
Right back at ya. Thanks for the review 👍
sphuber
force-pushed
the
fix/4165/base-restart-report-exit-code
branch
from
5d8f1d4 to
b87e2fd
Compare
If at the end of the method, `last_report` is defined, meaning at least one handler returned a report, it should return the corresponding exit code, except it was erroneously accessing that of the `report` variable, which could very well be `None` if the last called handler returned nothing.
sphuber
force-pushed
the
fix/4165/base-restart-report-exit-code
branch
from
b87e2fd to
f8a26ae
Compare
sphuber
removed
the
pr/blocked
Fixes #4165
If at the end of the method,
last_reportis defined, meaning at leastone handler returned a report, it should return the corresponding exit
code, except it was erroneously accessing that of the
reportvariable,which could very well be
Noneif the last called handler returnednothing.