Arvados

• ContainerRequest state: Committed
• Container state: Complete
• Container exit code: number != 0

In this example the container has

{
  "state": "Complete",
  "exit_code": 33
}

This state should be rendered in Workbench as a failure.

Fixed the issue at the container_work_unit model. Added related fixture and test @ 74b5dd2

74b5dd2b6

• Can you please update the container_work_unit -> state_label method something as follows? Basically, the get_combined already gets the value from the “other” object as needed …

    exit_code = get_combined(:exit_code)
    return "Failed" if (exit_code && exit_code != 0)
    get_combined(:state)

• Please consider adding “ exit_code: 0” to all other “Complete” container fixtures.

• Doesn’t hurt to expand the test "state_label should be Failed if container exit_code not 0" to also check that it is “not Failed” when exit code is 0 (even though it is implicitly tested elsewhere)

Above comments addressed @ 24eee13

Lucas: Looking at the test, I can’t really tell what it’s purpose is. I think it can be a bit more self explanatory. Something as follows?

    …
  ].each do |cr_fixture, state, exit_code|
    test “Completed ContainerRequest state = #{state} when exit_code = #{exit_code}” do
       …
       assert_equal exit_code, …
       assert_equal state, …

Rest LGTM. Thanks.

The test was written that way because I didn't want to add an exit_code accessor method just to support it, I now have updated the test as per your comments, but had to add that method to the model, you can check it at 8da182d .

• Lucas said: The test was written that way because I didn't want to add an exit_code accessor method

Yes, we need this. Also, please add "def exit_code" to work_unit.rb as well (this is to serve as the interface definition of all work units). We do not need to implement this method in all other work_unit typed classes; it will just default to null in the case of other object types.

• Nit. Now that you have this method, you can use it in the state_label method rather than repeating the logic here

   def state_label
-    exit_code = get_combined(:exit_code)
+    ec = exit_code

Thanks. LGTM

Merged master into this branch, resolved conflicts at 3678eda .
Running entire suite before merging into master.