-
Notifications
You must be signed in to change notification settings - Fork 230
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
WORLDSERVICE-346 - Remove amp from Homepage regex #12401
WORLDSERVICE-346 - Remove amp from Homepage regex #12401
Conversation
…hub.com:bbc/simorgh into WORLDSERVICE-346-remove-amp-from-homepage-regex
@@ -56,6 +56,7 @@ Object.keys(config) | |||
}); | |||
|
|||
paths | |||
.filter(path => path.indexOf('/', 1) !== -1) |
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.
Note for reviewers: We have added in this filter logic in order to keep only urls that contain a second forward-slash, excluding homepage url paths from being included in this set of amp tests.
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.
Open to suggestions on this one from other folk. We can't use pageType
here to determine whether or not to run, as 'specialFeatures' / cookie banner tests aren't broken down by page type. If there is a neater way to exclude homepages that would be good!
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.
@louisearchibald If everyone is happy to keep this logic, I'd add a comment in code above the .filter
function to indicate what its intended to do.
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.
I have a couple of ideas...
- add the homepage paths to the
urlsToExcludeFromAmpTests
array - but we need to be really careful about the value ofservice
here, because it's actually different for the variant services e.g.serbianCyr
/serbianLat
rather thanserbian
- so we need to to extract theserviceName
from the config file and use it instead.
So we could do something along the lines of:
const { variant, serviceName } = config[service];
const variantPath = variant === 'default' ? '' : `/${variant}`;
const homepagePath = `/${serviceName}${variantPath}`;
urlsToExcludeFromAmpTests.push(homepagePath);
- Have a named filter to exclude the homepage path - again it needs the correct service name from ☝️
const { variant, serviceName } = config[service];
const variantPath = variant === 'default' ? '' : `/${variant}`;
const isHomePagePath = path, serviceName => path === `/${serviceName}${variantPath}`;
paths.filter(path => !isHomePagePath(path, serviceName))
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.
Either of those look good? Just so long as that variant is captured correctly as you say.
The urls to exclude array is already in use, so maybe that option?
Is that ok with you @louisearchibald ?
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.
Yeah my preference was the urlsToExcludeFromAmpTests
array, because it is more readable than array filtering!
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.
Yep @amoore108 @karinathomasbbc , will make some changes and push up 👍
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 stuff, thanks both 🙏🏻
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.
Just thinking, this logic may be problematic:
if (!urlsToExcludeFromAmpTests.some(url => path.includes(url)))
The path.includes(url)
will capture all the URLs for the test suite, e.g. /thai
, /thai/articles/xxxxx
etc because it includes the string /thai
, so it won't just skip the homepage route.
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.
Ah... so it would. Maybe the other option then?
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.
Thanks for the amendments. I tested locally and all looks good 👍
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.
Nice work, thanks for this!
@@ -45,6 +45,8 @@ Object.keys(config) | |||
.filter(service => serviceFilter(service)) | |||
.forEach(service => { | |||
const { variant } = config[service]; | |||
const variantPath = variant === 'default' ? '' : `/${variant}`; | |||
const isHomePagePath = path => path === `/${service}${variantPath}`; |
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.
🎏 What happens when we have serbian / uzbek / zhongwen configured?
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.
We'll need to tackle that if/when it happens 😅 Lou did point this out during our discussions on a solution, so we'll need some extra logic to cater for the serbianCyr
naming etc.
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.
Yeah I checked the CD pipeline and scheduled E2Es and none of the variant services are configured in the cypress settings file for these cookie banner tests. If we were to add them, then we would need to fix it then, but don't need to worry about it at the moment. (We've had to workaround this annoying configuration many times in the Cypress tests 🙄)
Resolves JIRA [346]
Overall changes
Updates the regex for Homepages by removing the
.amp
segment from the pattern, since we have now removed all amp routes for this pagetype. Amp test files have also been deleted.Code changes
getHomePageRegex
function to remove any amp references.