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

Add functional test for delete collection then post scenario #62

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mostlygeek
Copy link
Contributor

@mostlygeek mostlygeek commented Jun 1, 2017

A bug was discovered in gosync (issue #169) where a DELETE of a
collection followed by a POST had the wrong behaviour. Add
test_delete_collection_then_post to ensure all servers conform to the
expected behaviour by clients.

The gosync issue #169 contains more context on this issue.

A bug was discovered in gosync (issue #169) where a DELETE of a
collection followed by a POST had the wrong behaviour. Add
test_delete_collection_then_post to ensure all servers conform to the
expected behaviour by clients.
res = resp.json
self.assertTrue("col2" in res)

infoBefore = self.app.get(self.root + '/info/collections')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different from the previous resp on L588?


# post BSOs again with X-I-U-S == 0
self.app.post_json(
self.root + '/storage/col2', bsos,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This could fit on the previous line

items = self.app.get(self.root + '/storage/col2').json
self.assertEquals(len(items), 0)
self.app.post_json(
self.root + '/storage/col2', bsos,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This could fit on the previous line

@Natim Natim force-pushed the improve-col-delete-tests branch from 086ccb9 to e3f9bc5 Compare June 2, 2017 05:47
Copy link
Contributor

@rfk rfk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM apart from test nits, I'll push some tweaks and merge.

self.assertTrue("X-Last-Modified" in infoBefore.headers)
self.assertTrue("X-Last-Modified" in infoAfter1.headers)
self.assertNotEqual(infoBefore.headers["X-Last-Modified"],
infoAfter1.headers["X-Last-Modified"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to assert that after is greater than before, not just that they're unequal.

headers=[('X-If-Unmodified-Since', "0.00")])
infoAfter3 = self.app.get(self.root + '/info/collections')
self.assertNotEqual(infoAfter3.headers["X-Last-Modified"],
infoAfter2.headers["X-Last-Modified"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here re: greater-than

@rfk
Copy link
Contributor

rfk commented Jun 2, 2017

This LGTM apart from test nits, I'll push some tweaks and merge.

Oh snap, I think adding a strict ordering check in the tests has uncovered a bug in the python server. I will see about a fix and push for re-review.

@rfk
Copy link
Contributor

rfk commented Jun 2, 2017

The python server is doing this to determine the overall storage timestamp:

STORAGE_TIMESTAMP = "SELECT MAX(last_modified) FROM user_collections "\
                    "WHERE userid=:userid"

Which is obviously wrong, for the same reason that just taking the max modified time of a BSO is wrong for a collection.

@rfk rfk self-assigned this Jun 2, 2017
@rfk
Copy link
Contributor

rfk commented Jan 14, 2019

@bbangert I'd be curious whether the new spanner backend can or could pass this additional functional test, which the python backend currently doesn't.

@bbangert
Copy link
Member

Flagging this for @pjenvey lest we forget it forever.

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

Successfully merging this pull request may close these issues.

None yet

4 participants