-
Notifications
You must be signed in to change notification settings - Fork 36
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
Home Page Test Scripts added for all locales #348
Conversation
tests/feds/homePageSanity.test.js
Outdated
|
||
test.describe('Test Suite for Home Page Components', () => { | ||
|
||
const validateLocalePage = async (page, baseURL, featureIndex, locale) => { |
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.
@PavanKumarN8 Why define this here rather than the page object class?
cc @sigadamvenkata @JackySun9 @skumar09
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.
agree to move into page object
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.
Sure, I will move it into page object.
tests/feds/homePageSanity.test.js
Outdated
}); | ||
}; | ||
|
||
test(`${features[13].name}, ${features[13].tags}`, async ({ page, baseURL }) => { |
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.
Any reason why this isn't in order? It goes from 13 to 25 to 57.
cc @PavanKumarN8
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 first tested tier 1 locales - JP, IT, US, UK, FR, & DE.
tests/feds/homePageSanity.test.js
Outdated
await validateLocalePage(page, baseURL, 13, "United States"); | ||
}); | ||
|
||
test(`${features[25].name}, ${features[25].tags}`, async ({ page, baseURL }) => { |
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.
Add the country to your spec like:
{ tcid: '0', name: '@ArgentinaHomePageGnavCheck', path: '/ar/?georouting=off', tags: '@gnavsanity @gnavhomeargentina', country: 'France', },
You can then do
features.forEach((props) => { test(
${props.name}, $props.tags}, async ({ page, baseURL }) => { await validateLocalePage(page, baseURL, props.tcid, props.country); }); }
This would help you bring down your 400 lines in this test file.
cc @PavanKumarN8 @JackySun9 @sigadamvenkata @skumar09
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.
Sure, I will update it. Thanks, for the solution.
features: [ | ||
{ | ||
tcid: '0', | ||
name: '@ArgentinaHomePageGnavCheck', |
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.
can we have only one here, then use homePage.json to get the path?
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 mean the path is not static like this, it is dynamic and load per homePage.json
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.
Sure, I will try that.
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.
Hi @JackySun9, we are using this file to uniquely identify the locale, and all the values in the feature file are different from one another. However, I am unable to optimize it. If you have a solution, please help with this, and I am happy to modify it accordingly.
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.
yes, I will try it later
tests/feds/homePageSanity.test.js
Outdated
|
||
test.describe('Test Suite for Home Page Components', () => { | ||
|
||
const validateLocalePage = async (page, baseURL, featureIndex, locale) => { |
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.
agree to move into page object
{ | ||
"locales": { | ||
"United States": { | ||
"unavElements": [ |
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 have a lot of duplications here, can we have a common one, then just add difference ones if existing for other locales, make this clear and short
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.
Okay, sure.
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.
Hi @JackySun9, I agree that we have duplicate values, but the sequence and options will differ for each locale. Attempting to store common options and optimize them results in script failures. If you have a solution, please assist with this.
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.
Yes, I will try this later
tests/feds/homePageSanity.test.js
Outdated
await validateLocalePage(page, baseURL, 25, "France"); | ||
}); | ||
|
||
test(`${features[57].name}, ${features[57].tags}`, async ({ page, baseURL }) => { |
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.
If we use dynamic path, we can just need one test here
You just resolve other feedbacks, then can checkin, I will take a look later |
@Dli3 do you have any feedback on this? |
@PavanKumarN8 @sigadamvenkata This PR broke the eslint workflow. Please check the checks before merging. Also, @PavanKumarN8 looks like you need to sign the Adobe CLA. |
Added Home Page Test Scripts for all locales
CC: @sigadamvenkata