Bug #3889

[API] [Workbench] Refactor any functional tests that call multiple actions into either integration tests or independent functional tests.

Added by Tom Clegg almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
API
Target version:
Start date:
09/21/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Description

Example: "admin can create collection with unsigned manifest".

If multiple actions are called from a functional test, they all share a single controller object. This means after the first action, the controller object has some state left over from previous actions. This is likely to change the behavior of the actions and thereby make the tests ineffective.

If possible, when running in test mode we should detect this sort of controller re-use and raise an exception, so we don't unwittingly create more bugs like this in the future.

Workbench-functional-failures (2.38 KB) Workbench-functional-failures Radhika Chippada, 09/21/2014 01:43 AM
API-functional-failures (6.67 KB) API-functional-failures Radhika Chippada, 09/25/2014 06:03 PM

Subtasks

Task #3940: Raise exception when multiple actions are performed on the controller in functional testingResolvedRadhika Chippada

Task #3941: Identify API functional tests that are performing multiple actions on a single controller. Either fix them or convert them into integration tests.ResolvedRadhika Chippada

Task #3942: Identify Workbench functional tests that are performing multiple actions on a single controller. Either fix them or convert them into integration tests.ResolvedRadhika Chippada

Task #3943: Review branch: 3889-functional-testing (raising exception on controller reuse only)ResolvedRadhika Chippada

Task #4001: Review branch: 3889-functional-testingResolvedTom Clegg

Associated revisions

Revision 8a833d4b
Added by Radhika Chippada almost 5 years ago

refs #3889: Merge monkey patch code updates to detect tests that are reusing ActionController::TestCase.
For now, print warning instead of raising error. After the tests are corrected, we can change this behavior to raise an error.
Merge branch '3889-functional-testing'

Revision f6d1dd80
Added by Radhika Chippada almost 5 years ago

closes #3889
Merge branch '3889-functional-testing'

History

#1 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg almost 5 years ago

  • Target version changed from Arvados Future Sprints to 2014-10-08 sprint

#3 Updated by Radhika Chippada almost 5 years ago

  • Assigned To set to Radhika Chippada

#4 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)
  • Category set to API

#5 Updated by Radhika Chippada almost 5 years ago

  • Status changed from New to In Progress

Update test_helper.rb to monkey patch ActionController::TestCase for GET / POST / PUT / DELETE methods.

#6 Updated by Radhika Chippada almost 5 years ago

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)

#7 Updated by Radhika Chippada almost 5 years ago

#8 Updated by Tom Clegg almost 5 years ago

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...

#9 Updated by Radhika Chippada almost 5 years ago

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

#10 Updated by Radhika Chippada almost 5 years ago

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.

#11 Updated by Radhika Chippada almost 5 years ago

  • File deleted (API-functional-failures)

#12 Updated by Radhika Chippada almost 5 years ago

#13 Updated by Radhika Chippada almost 5 years ago

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.

#14 Updated by Tom Clegg almost 5 years ago

  • 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.

#15 Updated by Radhika Chippada almost 5 years ago

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.

#16 Updated by Tom Clegg almost 5 years ago

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!

#17 Updated by Radhika Chippada almost 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:f6d1dd8018f7facf8e4c1f3c82ce777fec1d5a6a.

Also available in: Atom PDF