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.

Sign up for GitHub

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 failing specs due to new wikipageviews API behavior when no data available #5513

Merged
merged 3 commits into from
Oct 20, 2023
+10 −8
Merged

Fix failing specs due to new wikipageviews API behavior when no data available #5513

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
673c3f3
Avoid raising PageviewApiError when getting no data available respons…
gabina Oct 18, 2023
2732df4
Fix spec for article that exists but has no views by traveling to a g…
gabina Oct 19, 2023
41a142f
Rely only on the status being 404 status to return no results in wiki…
gabina Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/wiki_pageviews.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'])
Copy link
Member

Choose a reason for hiding this comment

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

Are there still also requests that return this error type, or did all 404's become about:blank?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

return no_results if no_data_available_response?(data)
raise PageviewApiError, response
end

def no_results
{}
end

# 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 '\
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

'for those date(s), or the project you asked for is not loaded yet.'\
' Please check documentation for more information.'
response['status'] == 404 && response['detail'] == no_data_avialable_detail
end

def wiki_url_param
# Wikidata works with either "www.wikidata" or just "wikidata", but not ".wikidata"
@wiki.language ? "#{@wiki.language}.#{@wiki.project}" : @wiki.project
Expand Down
2 changes: 2 additions & 0 deletions spec/lib/wiki_pageviews_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@
let(:title) { 'Voyages,_aventures_et_combats/Chapitre_18' }
let(:subject) { described_class.new(article).average_views }

before { travel_to Date.new(2023, 10, 18) }

Comment on lines +162 to +163
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

it 'returns 0' do
VCR.use_cassette 'wiki_pageviews/404_handling' do
expect(subject).to eq(0)
Expand Down
Loading