-
Notifications
You must be signed in to change notification settings - Fork 3
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: verify caching headers for dataset-metadata #204
Conversation
Codecov Report
@@ Coverage Diff @@
## main #204 +/- ##
=======================================
Coverage 70.42% 70.42%
=======================================
Files 132 132
Lines 10028 10028
=======================================
Hits 7062 7062
Misses 2966 2966
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see E2E testing in place! A few suggestions for your review.
def verify_response(self, result): | ||
self.assertEqual(result.status_code, HTTPStatus.OK) | ||
self.assertEqual(result.headers["Content-Type"], "application/json") | ||
self.assertTrue(result.cache_control.public) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope for this test-focused PR, but if we're not allowing caching I don't think public
is useful; if you agree, can you make a cool down story to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean remove public from the cache headers in dataset-metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the improvements!
url_base = "/".join( | ||
[self.api_url_base, "cellxgene", "s3_uri", quote(quote(body, safe=""), safe=""), "api/v0.3"] | ||
) | ||
endpoints = ["config", "schema", "colors", "genesets", "layout/obs", "annotations/obs", "annotations/var"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Reviewers
Functional: @atolopko-czi
Readability:
related to #200
Changes