Fix failing specs due to new wikipageviews API behavior when no data available #5513
Conversation
…e due to new wikipageviews api behavior. This change should fix failing wikipageviews specs.
|
CI failed, will check Edit: the rerun has only one failure. Edit: it's passing now |
…iven day. This is because otherwise the test gets broken if someone visits the article.
| before { travel_to Date.new(2023, 10, 18) } | ||
|
|
There was a problem hiding this comment.
I had to add this because it looks like I visited the site yesterday so I broke the spec.
I noticed that because the build for PR #5498 failed with:
3) WikiPageviews.average_views_for_article for an article that exist but has no view data returns 0
Failure/Error: expect(subject).to eq(0)
expected: 0
got: 1.0
This way it should be more robust.
|
@ragesoss this PR is ready for review ( I don't think I can set you as reviewer) |
lib/wiki_pageviews.rb
Outdated
| # As of October 2023, we started to see 404 not found responses with about:blank type | ||
| # and a specific detail when handling requests for which no data is available | ||
| def no_data_available_response?(response) | ||
| no_data_avialable_detail = 'The date(s) you used are valid, but we either do not have data '\ |
There was a problem hiding this comment.
I think relying on the exact 'detail' message here is going to be too fragile. Is this always the message when type is about:blank? I think the type is less likely to change than the text of the 'detail' message, which could get updated wording at any point.
There was a problem hiding this comment.
At first glance, I thought that about:blankas type was a kind of bug on the API side, as I didn't find any specific documentation for that and it wasn't pretty meaningful for me. That's why I decided to rely on the detail instead, I thought that maybe the type would change in the near future. However, I think we can rely on the status as you mentioned in your other comment.
lib/wiki_pageviews.rb
Outdated
| @@ -101,13 +101,23 @@ def parse_results(response) | |||
| return data['items'] if data['items'] | |||
| # As of October 2017, the data type is https://www.mediawiki.org/wiki/HyperSwitch/errors/not_found | |||
| return no_results if %r{errors/not_found}.match?(data['type']) | |||
There was a problem hiding this comment.
Are there still also requests that return this error type, or did all 404's become about:blank?
There was a problem hiding this comment.
Maybe it would be better to just rely on the 404 status and ignore type altogether? I imagine that any case of a 404 should be treated the same way by the Dashboard, and it looks like that's the only error mode that we've ever handled anyway.
There was a problem hiding this comment.
hmm good point. I wasn't able to reproduce any request returning https://www.mediawiki.org/wiki/HyperSwitch/errors/not_found as type. If I understand correctly, this request should be one of those that returned that type. So I think that your point about only relying on the 404 status makes total sense. Done on 41a142f. Two specs are failing but I guess those are flaky tests?
| VCR.use_cassette 'wiki_pageviews/views_for_article' do | ||
| expect { subject }.not_to raise_error |
There was a problem hiding this comment.
I don't think that stubbing the request makes sense now. Instead of that, I set start and end date out of the "allowed data range"
|
LGTM! |
gabina
deleted the
update-wikipageviews-to-fix-specs-for-new-api-behavior
branch
What this PR does
The behavior of the wikipageviews API has changed when handling requests for which no data is available, causing some of our specs to fail. This PR starts managing this new behavior to fix our specs.
It basically adds a new
no_data_available_response?method to identify these specific responses.Read more details in the proper issue.
Closes #5508
Screenshots
Open questions and concerns