-
Notifications
You must be signed in to change notification settings - Fork 6
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: ignore null items in API responses #240
Conversation
- configure the talk client to use the production API for tests. - test a talk production search that is known to break the API client. - ignore null response data, so that the tests pass.
@mcbouslog this fixes a bug that Alisa found earlier this week. |
eee5104
to
4a264d1
Compare
45ab170
to
43bcf0e
Compare
Now with mocks, so that the tests aren't running searches against the live API. The mocks are simply JSON snapshots from real Talk searches, including the search that's broken. Includes a real Talk production search query that only returns |
zooniverse/Panoptes-Front-End#7071 installs this version of the client in PFE. It needs to be deployed to staging, but here's a link to one of the broken searches: https://pr-7071.pfe-preview.zooniverse.org/talk/search?query=depression&env=production |
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.
Changes look good, mock talkClient and testing additions look great.
Confirmed using zooniverse/Panoptes-Front-End#7071 locally that existing error message "There was an error with your search. Please try again." is fixed with these changes 👍 .
On page 3 of the results (https://local.zooniverse.org:3735/talk/search?env=production&page=3&query=depression) the pagination shows a page 4 and 5, which are the null
response data pages, which is a little confusing, but that's ok and beyond the scope of these changes, just noting for myself.
There's definitely something weird going on with that search. After the first 29 results, the remaining 13 are all null. |
nock
, using snapshots of actual API responses.