codementordev / cm-quiz

Review your quiz
2 stars 8 forks source link

Review::DeleteIdea always failing #5

Open cawel opened 6 years ago

cawel commented 6 years ago

In ReviewQuiz, I don't see how the Review::DeleteIdea test can be successful, as it expects the database to not contain any Ideas at the very end of the test:

res = send_get_ideas_request(jwt: jwt)
res_hash = JSON.parse(res.body)
expect(res_hash.size).to eq(0)

However, some tests have run earlier and created Ideas in the database without removing them. Here is an example from Review::GetIdeas:

idea_payloads = 3.times.map do |i|
  Factory::Idea.new({
    project_api: @project_api,
    jwt: jwt,
    idea_params: {
      confidence: (3 + i) % 10 + 1
    }
  }).create
end

As a result, since the Review::DeleteIdea test starts while the database contains Ideas, naturally the call to /ideas at the end of Review::DeleteIdea will return some Ideas.

iamsudip commented 6 years ago

Same here. Running multiple times. For a clean DB, only delete_idea fails, after running multiple times get and delete idea fails.

iamsudip commented 6 years ago

Actually I have figured out the issue. We can blame the problem statement, can confirm no issues with test cases. :)

hwhelchel commented 6 years ago

I ran into this as well. You could request the list of ideas twice. Once before the deletion and once after.

Then the expectation becomes

expect(res_hash.size).to eq(before_res_hash.size - 1)

Which should pass on your first clean run. However, on the third run, this will now break due to the pagination logic. If you have greater than 10 records, and you delete one, you'll still get 10 records back from the server the next time around causing an issue.

ocowchun commented 6 years ago

Hello guys, I think the test should be ok to run if you implement GET /ideas correctly. Because the GET /ideas should only return user's idea. In the delete idea test we create one idea and delete it, so it should be 0 if both delete_idea and get_ideas implement correctly.

wasifhossain commented 5 years ago

@ocowchun you are right, but stating your point on the problem specification would help the candidates a lot, e.g. establishing user-idea association and using authorization to allow access to a user's own ideas only.

if you implement GET /ideas correctly

The fact is that when a developer tests the app without this tool, e.g. using Postman, he will access the endpoints (get/create/update/delete) using a token obtained from a single user. But this tool creates a new user before making a call to each endpoint, and so the tool will work only by following your point.

Nevertheless, I found the logic behind asserting the deletion of an idea resource to be buggy and so I have made a PR https://github.com/codementordev/cm-quiz/pull/7, which should make the tests pass even without implementing the user-idea association.

nrmiller commented 5 years ago

I'm not a Ruby programmer, but the issue with deleting always failing appears to be here:

delete_idea.rb:

def send_get_ideas_request(jwt:)
    options = {
      headers: {
        'x-access-token' => jwt
      }
    }

    @project_api.request(:get, "/ideas", options
end

The query parameter (page) is not provided, but the API documentation requires the page to be greater/equal to 1.

Comparing with get_ideas.rb:

def send_get_ideas_request(jwt:, page: 1)
    @options = {
      headers: {
        'x-access-token' => jwt
      },
      query: {
        page: page
      }
    }

    @project_api.request(@verb, @path, @options)
end

Hope this helps!