Skip to content

Commit

Permalink
Fix bug in BaseRestartWorkChain.inspect_process (#4166)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sphuber authored Jun 9, 2020
1 parent 6f029df commit 5b89d6f
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 12 deletions.
2 changes: 1 addition & 1 deletion aiida/engine/processes/workchains/restart.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ def inspect_process(self): # pylint: disable=inconsistent-return-statements,too

self.report(template.format(*report_args))

return report.exit_code
return last_report.exit_code

# Otherwise the process was successful and no handler returned anything so we consider the work done
self.ctx.is_finished = True
Expand Down
30 changes: 19 additions & 11 deletions tests/engine/processes/workchains/test_restart.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
# For further information please visit http://www.aiida.net #
###########################################################################
"""Tests for `aiida.engine.processes.workchains.restart` module."""
# pylint: disable=invalid-name
# pylint: disable=invalid-name,inconsistent-return-statements,no-self-use,no-member
import pytest

from aiida.engine import CalcJob, BaseRestartWorkChain, process_handler, ProcessState, ProcessHandlerReport, ExitCode
Expand All @@ -19,12 +19,16 @@ class SomeWorkChain(BaseRestartWorkChain):

_process_class = CalcJob

@process_handler()
def handler_a(self, node): # pylint: disable=inconsistent-return-statements,no-self-use
@process_handler(priority=200)
def handler_a(self, node):
if node.exit_status == 400:
return ProcessHandlerReport()
return ProcessHandlerReport(do_break=False, exit_code=ExitCode(418, 'IMATEAPOT'))

def not_a_handler(self, node):
@process_handler(priority=100)
def handler_b(self, _):
return

def not_a_handler(self, _):
pass


Expand All @@ -37,7 +41,7 @@ def test_is_process_handler():

def test_get_process_handler():
"""Test the `BaseRestartWorkChain.get_process_handlers` class method."""
assert [handler.__name__ for handler in SomeWorkChain.get_process_handlers()] == ['handler_a']
assert [handler.__name__ for handler in SomeWorkChain.get_process_handlers()] == ['handler_a', 'handler_b']


@pytest.mark.usefixtures('clear_database_before_test')
Expand All @@ -46,7 +50,7 @@ def test_excepted_process(generate_work_chain, generate_calculation_node):
process = generate_work_chain(SomeWorkChain, {})
process.setup()
process.ctx.children = [generate_calculation_node(ProcessState.EXCEPTED)]
assert process.inspect_process() == BaseRestartWorkChain.exit_codes.ERROR_SUB_PROCESS_EXCEPTED # pylint: disable=no-member
assert process.inspect_process() == BaseRestartWorkChain.exit_codes.ERROR_SUB_PROCESS_EXCEPTED


@pytest.mark.usefixtures('clear_database_before_test')
Expand All @@ -55,7 +59,7 @@ def test_killed_process(generate_work_chain, generate_calculation_node):
process = generate_work_chain(SomeWorkChain, {})
process.setup()
process.ctx.children = [generate_calculation_node(ProcessState.KILLED)]
assert process.inspect_process() == BaseRestartWorkChain.exit_codes.ERROR_SUB_PROCESS_KILLED # pylint: disable=no-member
assert process.inspect_process() == BaseRestartWorkChain.exit_codes.ERROR_SUB_PROCESS_KILLED


@pytest.mark.usefixtures('clear_database_before_test')
Expand Down Expand Up @@ -94,13 +98,17 @@ def test_unhandled_reset_after_handled(generate_work_chain, generate_calculation
"""Test `ctx.unhandled_failure` is reset to `False` in `inspect_process` after a handled failed process."""
process = generate_work_chain(SomeWorkChain, {})
process.setup()
process.ctx.children = [generate_calculation_node(exit_status=0)]
process.ctx.children = [generate_calculation_node(exit_status=300)]
assert process.inspect_process() is None
assert process.ctx.unhandled_failure is False
assert process.ctx.unhandled_failure is True

# 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

0 comments on commit 5b89d6f

Please sign in to comment.