Arvados

• The following update in test/unit/container_request_test.rb caused a concern

   test "Container request commit" do
     set_user_from_auth :active
-    cr = create_minimal_req!(runtime_constraints: {"vcpus" => [2,3]})
+    cr = create_minimal_req!(runtime_constraints: {"vcpus" => 2, "ram" => 30})

In container_request.rb, the method runtime_constraints_for_container uses the first value in the array for a field. Hence, I think the validator should check whether an array or an integer is given for vcpus. If an array is provided, should raise error if the first value is not an integer (seems like the second value is ignored; otherwise, you might want to ensure that it is an integer as well). Also, please revert back this test update.

• In test "Container request constraints must include valid vcpus and ram fields when committed", can you please add a case where the CR is modified (the existing tests only test create path)

• Also, please add a test with array for vcpus and non-integer value(s).

Thanks.

Radhika Chippada wrote:

In container_request.rb, the method runtime_constraints_for_container uses the first value in the array for a field. Hence, I think the validator should check whether an array or an integer is given for vcpus. If an array is provided, should raise error if the first value is not an integer (seems like the second value is ignored; otherwise, you might want to ensure that it is an integer as well). Also, please revert back this test update.

Lucas noticed this and asked about this on IRC. Tom and I discussed it with him, and we agreed that at this time, we don't want to provide any support for value ranges (two-element arrays) in these constraints at this time. None of the Crunch v2 implementations currently support them either, so limiting the values to integers for now will ensure that container requests have values that dispatchers actually support, and we can permit ranges in the validations when the dispatchers are extended to support that. The diff you pasted follows up on that discussion fine, AFAICT.

97c669426fd4a086883d1cf666043af17f99f758

Added tests on container request update. Also added tests for both successful create & update cases

Thanks for the update. Just a few suggestions.

• Rather than create a new CR for each update test, you can reuse the object doing something like:

-    cr = create_minimal_req!(state: "Uncommitted", priority: 1)
-    cr.save!
+    cr = ContainerRequest.find_by_uuid cr.uuid
     cr.state = "Committed" 

• It would be easier to read the code if cr.state is also within the raises block:

-    cr.state = "Committed" 
     assert_raises(ActiveRecord::RecordInvalid) do
+      cr.state = "Committed" 
       cr.runtime_constraints = {"vcpus" => 1}
       cr.save!
     end

• It is desirable to include update tests using update_attribute:

+    cr = ContainerRequest.find_by_uuid cr.uuid
+    cr.state = "Committed" 
+    assert_raises(ActiveRecord::RecordInvalid) do
+      cr.update_attributes!(runtime_constraints: {"vcpus" => 1, "ram" => "123"})
+    end

b6a922bd21b2c9da8861de4c84c2a2ab25666ef2

Thanks for the suggestions, I updated the code and also eliminated repeated lines while trying to maintain readability.

Lucas, this is SO MUCH better. Thanks. Just a couple more observations and some nitpicks.

• Please convert the update_attributes test at line 85 (or add another block similar to "validations on update") to failing tests. Our goal here is to test that it fails.

• Can you please add another invalid_constraint with a string value, something like {"vcpus" => "1", "ram" => "123"}?

• We don't need the following at line 68 because we have another test that is invoking create_minimal_req with state "Committed" already, correct?

    cr = create_minimal_req!(state: "Committed", priority: 1)    
    cr.save!

• Can you please just throw in a line separator between blocks (for example, above the comment "Validations on update" etc) to improve readability

Thanks.

Radhika Chippada wrote:

Lucas, this is SO MUCH better. Thanks. Just a couple more observations and some nitpicks.

• Please convert the update_attributes test at line 85 (or add another block similar to "validations on update") to failing tests. Our goal here is to test that it fails.

I'm not sure if I understand correctly. The failing tests are the ones on the invalid_constraint blocks, the idea on this other one was to test a successful case, added an assert to make it more explicit, but not sure if I'm missing something else.

• Can you please add another invalid_constraint with a string value, something like {"vcpus" => "1", "ram" => "123"}?

Updated.

• We don't need the following at line 68 because we have another test that is invoking create_minimal_req with state "Committed" already, correct?

Yes, that test case was already included on the "Container request commit" test so, I deleted it.

• Can you please just throw in a line separator between blocks (for example, above the comment "Validations on update" etc) to improve readability

Done.

Thanks for the observations! You can check the diff @ b0993528fa57bb1866bbd6a2b19822cc75010a66

73c06c1a5af0e62fb937304fa6bee19225b9d7f6

Test code re-styling as per Radhika's suggestions, thanks!

Lucas: this is much better. Thanks.

LGTM

4a1dfd3c16896a873fa32becbdd264aa10ada7af

Added runtime constraints to fixtures.

Lucas, thanks for the update.

LGTM