Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #empty? responding correctly to empty responses #36

Merged
merged 1 commit into from
Aug 24, 2016

Conversation

pye2k
Copy link
Contributor

@pye2k pye2k commented Aug 24, 2016

Pull request for #35.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Coverage remained the same at 97.638% when pulling e669bdb on pye2k:master into 7868f8b on aws:master.

@awood45
Copy link
Member

awood45 commented Aug 24, 2016

I'm checking this against some integration test cases I have now. Pagination in the query/scan operations could produce new edge cases to test for.

@awood45
Copy link
Member

awood45 commented Aug 24, 2016

Actually, I'm pretty sure for a couple reasons that the test case I have in mind is both impossible (service won't do it) and already covered, but let's add this to make sure we handle it gracefully, including going forward. Test case is like so:

it "is not empty" do
  stub_client.stub_responses(:scan, truncated_resp, resp_empty)
  c = ItemCollection.new(
    :scan,
    { table_name: "TestTable" },
      model,
      stub_client
  )
  expect(c.empty?).to be_falsy
end

You'll need to expand the scope of where truncated_resp is available to test this. This ensures that if we aren't only checking the last page. With the current implementation, I don't think we even get to the second client call.

@awood45
Copy link
Member

awood45 commented Aug 24, 2016

Actually, I'll just add that before pushing a release. Thanks!

@awood45 awood45 merged commit 206747c into aws:master Aug 24, 2016
@pye2k
Copy link
Contributor Author

pye2k commented Aug 24, 2016

👍

@awood45
Copy link
Member

awood45 commented Aug 24, 2016

I added additional support for an edge case related to empty pages followed by pages with content, which is possible in scan operations.

I also added documentation warnings, given that this can easily do full table scans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants