Arvados

Notes to reviewer of branch 3889-functional-testing (raising exception on controller reuse):

• I could not find a gem that detects functional tests that perform more than one action on a controller
• Hence, I used monkey patching to enhance GET / POST / PUT / DELETE methods to first check a counter and then raise an exception if controller is reused
• There are 20 API functional tests that are using this incorrect approach (see attachment for a list of these tests)
• There are 5 Workbench functional tests that are doing this (see attachment for a list of these tests)

At 768889a

This approach seems to work very well. It turns up many such errors. Apparently systematic checking was worthwhile!

Pushed 5fd423a on a 3889-functional-testing-TC branch with suggested improvements on the API side (didn't update the Workbench side yet, though):

• Global variable is unnecessary, can be an instance variable
• Use "setup" callback, instead of patching the run_callbacks method
• Use define_method in a loop instead of copying & pasting the same method
• Use super instead of alias/original_xxx
• Add patch to list of HTTP methods checked

I also changed "raise" to "print warning" -- this makes it possible to merge this branch before every last transgression is fixed. Thoughts?

Aside: We should probably be good citizens and publish this as the-gem-we-didn't-find-when-we-looked-for-it...

Tom, agree with you on printing a warning rather than raising an exception, and merging into master this portion of the implementation.

Tom, regarding your comment "Aside: We should probably be good citizens and publish this as the-gem-we-didn't-find-when-we-looked-for-it..." : Yes, this thought crossed my mind as well. We can discuss this at a leisurely manner and look into contributing this to the community as Arvados / Curoverse team contribution. Thanks.

Notes to reviewer: Please let me know if the following test updates are agreeable or not.

• apps/workbench/test/functional/pipeline_instances_controller_test.rb
  • Are the two pipeline instance fixtures I added meeting the desired test goals?

• services/api/test/functional/arvados/v1/groups_controller_test.rb

• services/api/test/functional/arvados/v1/jobs_controller_test.rb

• services/api/test/functional/arvados/v1/keep_disks_controller_test.rb

• I think, those I did not list here are - as far as I can tell, do not change the previous test behavior and are safe.

• In apps/workbench/test/functional/actions_controller_test.rb I suspect this line is superfluous since no further requests are made:

  • +    use_token :active

• In apps/workbench/test/functional/pipeline_instances_controller_test.rb instead of deleting the pipeline instance with a post (or leaving it sitting around) we could delete it directly:

  • -    post :destroy, {
    -      id: pi_uuid,
    -      format: :json
    -    }
    -    assert_response :success
    +    PipelineInstance.where(uuid: pi_uuid).delete_all
    

• In services/api/test/functional/arvados/v1/collections_controller_test.rb we might have lost the only "get :show, { id: portable_data_hash }" test (at least in this file).
  • Suggestion: split "admin can create collection with unsigned manifest" into two tests, one for the first half, which matches that title, and a one for the second half, "admin can get collection using portable data hash as id".
  • Either way, retrieved_collection = Collection.select ... is misleading: I think it would be better to say either response_collection = assigns(:object) or stored_collection = Collection.select ... depending on whether you want to test the response of the :create action or what got stored in the database.
  • Aside: does/should the :create action add permission signatures to manifest_text in its response here? This test doesn't seem to care.
• In services/api/test/functional/arvados/v1/groups_controller_test.rb I can't think of a good way to make sure we reach offset=items_available here. Also, the "receive item on multiple pages" test is ineffective now that the uuid_received hash is cleared before each page. Perhaps this one has to become an integration test?
• In services/api/test/functional/arvados/v1/jobs_controller_test.rb
  • I find the (old) "normalize output and log uuids when creating job" test confusing. It seems to verify that updating the log attribute doesn't cause the script_version attribute to change to a non-SHA1. Huh? (Maybe this was a copy/paste fail and should be removed from the 2nd (new) test now that we're actually testing what was probably intended before?)
  • If the new "unnormalized" test fixture is supposed to replace new_job in the old test, I think its log and output attributes should be normalized: the test was (and I figure should still be) whether a canonical hash+size value can be changed to a non-canonical hash+size+hint value via put :update. So the fixture should have hash+size and the put :update params should be hash+size+hint.
  • could be using json_response instead of JSON.parse(@response.body) here...
• In services/api/test/functional/arvados/v1/jobs_controller_test.rb
  • test "#{user} update a job without failing script_version check" should probably be called test "admin update job owner to #{user} without failing script_version check"
  • (I think the background here is that the fixture's owner_uuid can't be both of those, so if we test changing it to A and changing it to B, we're certain the update didn't succeed merely because no update needed to happen at all. Perhaps this should be explained in a comment?)
• In services/api/test/functional/arvados/v1/logs_controller_test.rb
  • I think this test should be add back in the first "non-admins can create their own logs" test:

  •     assert_equal("test log", assigns(:object).summary,
                     "loaded wrong log after creation")
    

  • It might also be worth adding assert_equal("test log", Log.find_by_uuid(assigns(:object).uuid).summary, ...) to make sure it actually got into the database correctly.
  • (Having a summary assertion in the second test too doesn't hurt, of course.)
• In services/api/test/functional/arvados/v1/users_controller_test.rb
  • Should keep these assertions, just modify them:

  • -    assert_equal false, me['is_active']
    +    assert_equal false, users(:inactive).is_active
    

  • Wouldn't hurt to assert_equal false, json_response['is_active'] as well after the 403
  • Use existing json_response instead of doing response_body = JSON.parse(@response.body)
• In services/api/test/functional/arvados/v1/users_controller_test.rb
  • test "create user with user, vm and repo as input" used to (try to) verify that doing two "setup" operations with the same user/info is OK. I think this is worth testing -- do you agree? If an integration test is too much trouble, let's at least add a placeholder integration test and put "skip" in it to remind us. (Or do we already have such a test?)
  • test "setup user with an existing user email and check different object is created" makes me wonder (although I know it's out of scope if it's a bug): is that really what we want? What's so great about having two user records with the same email address -- which one gets used when the person eventually logs in? I would have expected we'd test that a second user account doesn't get added in this case, so perhaps I'm missing something.
• test "setup and unsetup user" seems important enough to become an integration test (as above re. placeholder if it's too much work now).

Phew. This was more work than I expected, but I already feel better knowing that we're not relying on weird side effects of controller reuse, and I think the tests are improved as a result.

Tom,
Addressed all your feedback comments and noting any exceptions here:

• "In apps/workbench/test/functional/actions_controller_test.rb I suspect this line is superfluous since no further requests are made" - This is needed to execute the next statement "Collection.select . . . "

• "test 'setup user with an existing user email and check different object is created' makes me wonder . . ." : When we implemented setup initially, we talked about it and you said it would be workbench's / client's responsibility to ensure email is not reused and we should create a new object in such a situation. In any case, as you said, it is out of scope for this story. If you prefer, let's create a bug and look into addressing this separately.

Thanks.

Radhika Chippada wrote:

Tom,
Addressed all your feedback comments and noting any exceptions here:

• "In apps/workbench/test/functional/actions_controller_test.rb I suspect this line is superfluous since no further requests are made" - This is needed to execute the next statement "Collection.select . . . "

Ah, my bad -- I was thinking of apiserver, which would be able to look up collections without auth.

• "test 'setup user with an existing user email and check different object is created' makes me wonder . . ." : When we implemented setup initially, we talked about it and you said it would be workbench's / client's responsibility to ensure email is not reused and we should create a new object in such a situation. In any case, as you said, it is out of scope for this story. If you prefer, let's create a bug and look into addressing this separately.

Hm, perhaps I was thinking of repo/login rather than email. In any case, yes, it's a separate story if it's anything at all.

LGTM, thanks!