-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
chore(#8159): Run our e2e standard test on minimum browser requirements #8663
chore(#8159): Run our e2e standard test on minimum browser requirements #8663
Conversation
Hi reviewers! I have divided the work on this issue into different pull requests, one per config/suite. This will make the work to review and merge gradual and manageable. This PR is only for standard config specs. Let me know what you think. Thanks! |
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.
Really exciting stuff.
A few suggestions inline.
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 this is awesome! Minor suggestions inline...
Co-authored-by: Gareth Bowen <gareth@medic.org>
…' of https://github.com/medic/cht-core into 8159-minimum-browser-requirements-chromedriver-standard merge with origin
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 work, thanks!
Some minor suggestions inline.
tests/utils/index.js
Outdated
|
||
const isMinimumChromeVersion = () => { | ||
// eslint-disable-next-line no-undef | ||
if (driver.capabilities.browserVersion.split('.').shift() === MINIMUM_BROWSER_VERSION) { |
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.
There's a little complexity here with all the different browser versions... we have:
driver.capabilities.browserVersion
MINIMUM_BROWSER_VERSION
process.env.CHROME_VERSION
I think it would be easier if we just don't use driver.capabilities.browserVersion
if we can, for example, use the env variable here too, eg:
if (driver.capabilities.browserVersion.split('.').shift() === MINIMUM_BROWSER_VERSION) { | |
if (process.env.CHROME_VERSION === MINIMUM_BROWSER_VERSION) { |
That way it's more consistent throughout the code.
This also means throughout the tests/e2e/wdio.conf.js
file you can use isMinimumChromeVersion
rather than getMinimumChromeVersion
which will simplify the code there a little too.
This might mean you can remove the getMinimumChromeVersion
function altogether...
What do you think?
ACTIVE_OPTION_LABEL, | ||
SMS_NOTE, | ||
FOLLOW_UP_SMS, | ||
}; |
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!
Just one point about naming conventions. Usually we use all caps for primitive constants, not functions, so I think FORM
, SMS_NOTE
and FOLLOW_UP_SMS
should be camel case.
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.
This is so great. Thanks for putting up with all my nitpicks!
Thank you, Gareth, for caring and all the detailed suggestions. |
Description
Use WebdriverIO Chromedriver service to run standard config e2e tests on minimum browser version, currently set at 90. Organize with a separate config and add command to CI pipeline.
#8159
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.