fix: ensure to remove tasks with errors from used deployments#548
Conversation
wvmcastro
left a comment
wvmcastro
left a comment
There was a problem hiding this comment.
Giving our time constraint, I would approve this with just the test documentation
There was a problem hiding this comment.
I think a few months from now knowing what these tests are actually testing will be a puzzle. I think it is worth documenting what the are testing and how. Moreover, if it wouldn't be too painful to write, I would like to see a test that checks @used_deployments before and after error resolution and asserts that the desired deployments were removed
There was a problem hiding this comment.
But they do test a typical behavior from #resolve. The issue before is that we assumed #resolve didnt need any tests due to being tested in higher level elsewhere, but this behavior actually had to be tested since it is specific to this set of parameters. I dont think they are specific to the errors we found, this code path is something that should be directly tested anyways.
Your suggestion is one that is very specific to this bug though, I dont agree that the unit tests should be this granular on this part of syskit.
There was a problem hiding this comment.
I actually prefer the system test, not depending on the internal implementation (which we are fixing actually)
jhonasiv
force-pushed
the
fix-deployment-of-tasks-outside-the-plan
branch
from
f92d14a to
a7071ec
Compare
There were two different scenarios where we got the same kind of error,
which looked like this:
NoMethodError: undefined method `[]' for nil:NilClass
/home/jhonas/dev/wetpaint/tools/roby/lib/roby/relations/space.rb:340:in `block (2 levels) in relation'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/runtime_network_adaptation.rb:454:in `adapt_existing_deployment'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/runtime_network_adaptation.rb:177:in `handle_required_deployment'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/runtime_network_adaptation.rb:129:in `block in finalize_used_deployments'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/runtime_network_adaptation.rb:127:in `each'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/runtime_network_adaptation.rb:127:in `finalize_used_deployments'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/runtime_network_adaptation.rb:66:in `finalize_deployed_tasks'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/runtime_network_adaptation.rb:40:in `apply'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/engine.rb:186:in `block in apply_deployed_network_to_plan'
/home/jhonas/dev/wetpaint/tools/roby/lib/roby/event_logging/mixin.rb:43:in `log_timepoint_group'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/engine.rb:179:in `apply_deployed_network_to_plan'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/engine.rb:613:in `block in apply_system_network_to_plan'
/home/jhonas/dev/wetpaint/tools/roby/lib/roby/event_logging/mixin.rb:43:in `log_timepoint_group'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/engine.rb:612:in `apply_system_network_to_plan'
/home/jhonas/dev/wetpaint/tools/syskit/lib/syskit/network_generation/engine.rb:587:in `resolve'
/home/jhonas/dev/wetpaint/tools/syskit/test/network_generation/test_engine.rb:737:in `block (4 levels) in <module:NetworkGeneration>'
That happened because we were removing tasks from the plan due to a
resolution error during the #apply_deployed_network_to_plan step, but we
never removed them from the used_deployments, therefore the system was
still trying to solve for tasks that were not in the plan anymore.
Passing the computed used deployments around was fragile, it had the need of being synchronized with the plan. Dump that need by always taking from the plan directly, if a task was cleaned up during the network resolution, the plan will reflect that.
jhonasiv
force-pushed
the
fix-deployment-of-tasks-outside-the-plan
branch
from
a7071ec to
8fcd2c5
Compare
At first glance this is not strictly necessary, but one can never be too sure.
jhonasiv
force-pushed
the
fix-deployment-of-tasks-outside-the-plan
branch
from
0f7ef9e to
36225ce
Compare
3 participants
There were two different scenarios where we got the same kind of error, which looked like this:
That happened because we were removing tasks from the plan due to a resolution error during the #apply_deployed_network_to_plan step, but we never removed them from the used_deployments, therefore the system was still trying to solve for tasks that were not in the plan anymore.