Supressednotificationstest #34
Conversation
tests when a user modifies the clinic profile, the supressednotification setting is preserved (true)
brian-tidepool
requested review from
clintonium-119,
vojtech-cerveny and
ginnyyadav
| '', | ||
| { | ||
| headers: { | ||
| 'Authorization': 'Basic YnJpYW4rdXBsb2FkNzg2d2ViMTMyN2NsaW5pYzM1YUB0aWRlcG9vbC5vcmc6dGlkZXBvb2w=', |
There was a problem hiding this comment.
issue: If we hide credentials into environment variables, we should do that for tokens as well. Or you can create this token from credentials, it should be possible. WDYT?
There was a problem hiding this comment.
This is not a token, those are base64 encoded credentials - consider rotating the password.
There was a problem hiding this comment.
Yup, I wrote it wrong 🤝 We can generated it from credentials saved in env and it should be enough, we don't need to rotate passwords IMHO
There was a problem hiding this comment.
This particular password has now been exposed to the entire world, given how this is a public repo. It may already be in https://archiveprogram.github.com/arctic-vault/ 😬
There was a problem hiding this comment.
i've changed the password for this account just now. ill look into rotation.
|
|
||
|
|
||
|
|
||
| const setNotification = async () => { |
There was a problem hiding this comment.
suggestion (if-minor): Some comment for this function would be cool.
Also you should specify, that it returns response.status, because it is kinda strange to see const data = await setNotification() === 200
Not ideal suggestion would be getNotificationStatus🤔 Not sure what it the best naming here :)
There was a problem hiding this comment.
-added comments to function
see commit e138f92
| ) | ||
| return response.status | ||
| } | ||
| const setNotificationRun = async () => { |
There was a problem hiding this comment.
nitpick (non-blocking): Name of function - you are asserting the notification status, so it should be clear from the function, what you do. WDYT?
There was a problem hiding this comment.
-clarification for function added in code comments
see commit e138f92
| clinicPatientList.setValue('@city', 'new'); | ||
| clinicPatientList.click('@clinicProfileSubmit'); | ||
| clinicPatientList.waitForElementNotVisible('@city', browser.globals.elementTimeout); | ||
| browser.suppressedNotificationsTrueCheck(); |
There was a problem hiding this comment.
suggestion: IMHO this is not good pattern. I am not fan of hiding assertion into function. If you will check the code in future, you will not see what you are checking, what is the assertion - you need to make the extra step to check the assertion. So I would rewrite it into this:
const isSuppresedNotificationPresent = browser.suppressedNotificationsTrueCheck(); // function needs to be rewritten
expect(isSuppresedNotificationPresent).to.be.true;WDYT?
There was a problem hiding this comment.
i've pushed a new commit 914dbef to address the above. the custom commands are refactored such that they return a result and the calling test checks the assertion.
There was a problem hiding this comment.
@vojtech-cerveny lmk if the changes look ok
| '', | ||
| { | ||
| headers: { | ||
| 'Authorization': 'Basic YnJpYW4rdXBsb2FkNzg2d2ViMTMyN2NsaW5pYzM1YUB0aWRlcG9vbC5vcmc6dGlkZXBvb2w=', |
There was a problem hiding this comment.
Let's not include credentials in PRs that are public to the world. This should come from injected environment variable at run-time (i.e. in the CI).
There was a problem hiding this comment.
In fact, the URL should also come from an environment variable as well, to make this code re-usable in other deployment environments.
There was a problem hiding this comment.
changed the password for this account for now. will look into injecting credentials into the environment variable.
…iables replaced credentials and environment url with environment variables that are read in at runtime (hardcoded values replaced in suppressedNotificationsTrueCheck.js, suppressedNotificationsTrue.js)
…pdates -renamed custom commands files -refactored so that returned a result and moved assertion to calling test -added @ROLE=button to various page objects -replaced === in various comparisons
…iables added -js doc style comments added for each sub function inside suppressed notification functions -hard coded values for clinic id and clinician id moved to global variables in nightwatch.conf.js -
Tests are failing due this error on localhost and also on CircleCI - https://app.circleci.com/pipelines/github/tidepool-org/WebUITests/1290/workflows/9cd52266-3461-48ca-98b7-e6c56ca64d04/jobs/1308/parallel-runs/0/steps/0-110 |
modifed file import in captureScreenshot.js so that it loads correctly
|
@vojtech-cerveny added commit 888e22f to address due to error thrown by captureElementScreenshot.js |
…nsFunctional add parallel tag to clinicianPageFunctional and suppressedNotificationsFunctional
first test for rpm stats file comparison test
Added functions for filters, rpm export button, and checks if sufficiency column meets criteria. Automated tests implemented checked as completed in test strategy spreadsheet: https://docs.google.com/spreadsheets/d/1gv5q_o5ghR_KMENYvsFmFxGVjKz020O9/edit?usp=sharing&ouid=117042773141856291352&rtpof=true&sd=true fix to start date and end date out of range click added logic to click previous month button if start date or end date was out of range in calendar view additional tests for filters last2days, last14days, last30days, today, and allranges split tests rpmStatsFunctionality file into multiple conditions
fixed all es lint errors excluding class-methods-use-this since it results in a nighwatch error
fileName param documentation added
add documentation to checkSufficiency function
Rpm Stats Automated Tests
|
LGTM |
Tests when editing a profile suppressed notifications are preserved.
(see related ticket: https://tidepool.atlassian.net/browse/BACK-3031 )
Would appreciate coding tips as well (i added some api calls in custom commands which doesn't seem typical for nightwatch js, but it seems usefu)