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

update SEO Test #18142

Merged
merged 10 commits into from Oct 14, 2021
Merged

update SEO Test #18142

merged 10 commits into from Oct 14, 2021

Conversation

peterhashair
Copy link
Contributor

Description:

Fixes: #17919
update SEO tests, It seems the print $_SERVER['REMOTE_ADDR'] doesn't give back the useful info.

bing-index expected non-zero rank, got [0], ip [127.0.0.1]

Review

update seo Test
@peterhashair peterhashair added this to the 4.6.0 milestone Oct 12, 2021
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 12, 2021
@tsteur
Copy link
Member

tsteur commented Oct 12, 2021

feel free to merge later @peterhashair 👍

Peter Zhang added 4 commits October 13, 2021 13:10
add ip and content for seo tests
update seo Tests
update seo tests
bing has anti crawler exception on SEO test
@peterhashair
Copy link
Contributor Author

peterhashair commented Oct 13, 2021

Did quite a lot of tests and research,

I think the SEO tests randomly failed is because Bing is a dynamic system with an anti crawler, our crawler exceeds the number of trying in a period of time it will return a page like this.

I Update bing getMetrics when there is no result instead of return 0, return a general error that should skip the tests

image

Peter Zhang added 3 commits October 13, 2021 18:47
fix the method
update error function
update Bing instead of update tests
@peterhashair peterhashair marked this pull request as ready for review October 13, 2021 07:01
@peterhashair peterhashair added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Oct 13, 2021
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

Nice find @peterhashair 👍

btw just in case this will fail randomly again in the HTTP request we might want to set $getExtendedInfo=true so we can check the HTTP response code. Did a quick check and bing says it would return an HTTP 429 if someone exceeds request per minute limit.

I didn't think we issue requests to bing that often but maybe other people on travis running these tests contribute to the quota too.

@@ -40,6 +40,8 @@ public function getMetrics($domain)

if (preg_match('#([0-9,\.]+) results#i', $response, $p)) {
$pageCount = NumberFormatter::getInstance()->formatNumber((int)str_replace(array(',', '.'), '', $p[1]));
} elseif (preg_match('#There are no results#i', $response, $p)) {
$pageCount = Piwik::translate('General_Error');
Copy link
Member

Choose a reason for hiding this comment

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

will this be shown in the UI? If so, we would maybe need to be a bit more specific as otherwise people will creates issues and contact support about it and ask for details.

@peterhashair
Copy link
Contributor Author

peterhashair commented Oct 13, 2021

@tsteur Bing doesn't return an error status, it loads through javascript. I don't think it will change too much. Only when someone did a website that does not exist will display an error page. It seems like maybe this error needs a little bit of update on the code. Every search engine looks different, let me know if we want to update this.
image

@tsteur
Copy link
Member

tsteur commented Oct 13, 2021

i see. Didn't realise others were already showing that simple error message. Possibly they don't error as often. We've had errors before on the cloud for bing so this happens more often. It be great to possibly use the same consistent message possibly. Even if it's just like an Error. Please try again later.

update SEO test
@peterhashair
Copy link
Contributor Author

@tsteur Sure, that should fix the SEO test and the error page.
image

@tsteur tsteur merged commit 39fb369 into 4.x-dev Oct 14, 2021
@tsteur tsteur deleted the m-17919-seo-ranking-tests branch October 14, 2021 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEO Ranking plugin not working - problem with ping? - Fix the random test failures
2 participants