-
Notifications
You must be signed in to change notification settings - Fork 4
PEPPER-587: RGP DSM playwright tests_assert creation of new participant by checking DSM #2048
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
Conversation
Changed from 'Pepper RGP' -> 'RGP' so that it'll be able to run no matter if used in dev, test, or staging
missed an eslint error
tweak from |
} | ||
|
||
async selectStudy(study: string, page: Page) { | ||
const studySelector = await new Select(page, { label: 'Select study' }); |
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.
Instead, you could use this way: this.navigation.selectStudy()
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.
Oh, anyway, this method is not used anywhere. Maybe remove it?
* @param opts | ||
*/ | ||
async logIn(opts: { email?: string; password?: string; waitForNavigation?: boolean } = {}) { | ||
await auth.login(this.page); |
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 think when you are on this page, you are already logged in, therefore I don't think this method is useful here. What do you think about removing it?
Please correct me if I'm wrong.
Usually, we just call the login()
method from beforeEach
.
It would be good if we considered later adding Login Page Object Model, which would handle all login-related things
await studySelector.selectOption(study); | ||
} | ||
|
||
async selectStudyMenuOption(menuOption: string) { |
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 not used as well
} | ||
} | ||
|
||
async selectCustomizeViewOption(columnGroup: string, columnName: string) { |
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 not used as well.
If you need to use customize view functionality anywhere, you can find it in CustomizeView class, which is located in the Filters class, that is available (according to DSM structure) in the ParticipantListPage class (POM).
@@ -20,6 +24,18 @@ export default class ParticipantListPage { | |||
await waitForNoSpinner(this.page); | |||
} | |||
|
|||
public async filterListByParticipantGUID(participantGUID: string): Promise<void> { | |||
await this.page.locator('text=Customize View >> button').click(); |
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.
Instead, you could use customize view functionality, which is available from the _filters
field. Like so:
const customizeViewPanel = this.filters.customizeViewPanel;
await customizeViewPanel.open();
await customizeViewPanel.selectColumns('Participant Columns', ['Participant ID']);
const searchPanel = this.filters.searchPanel;
await searchPanel.open();
await searchPanel.text('Participant ID', {textValue:participantGUID});
await searchPanel.search();
And later to make assertions.
What do you think?
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.
sounds good - will make the changes
import { Navigation } from 'lib/component/dsm/navigation/navigation'; | ||
import { StudyNav } from 'lib/component/dsm/navigation/enums/studyNav.enum'; | ||
import ParticipantListPage from 'pages/dsm/participantList-page'; | ||
import ParticipantPage from 'pages/dsm/participant-page'; |
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.
Unused import:
import ParticipantPage from 'pages/dsm/participant-page';
await this.navigation.selectFromStudy<ParticipantListPage>(StudyNav.PARTICIPANT_LIST); | ||
await this.participantList.waitForReady(); | ||
await this.participantList.assertPageTitle(); | ||
await this.participantList.waitForReady(); |
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.
async waitForReady(): Promise<void> { | ||
await this.page.waitForFunction(() => document.title === 'DDP Study Management'); | ||
} |
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.
FYI, document.title === 'DDP Study Managment'
tells me current page is the expected page. But it doesn't checks page is fully loaded and ready for user interaction. Just wanted you to know this, it's okay to leave it as is.
/** | ||
* Data Study Manager (DSM) page base | ||
*/ | ||
export abstract class DSMPageBase extends PageBase { |
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.
👍🏻👍🏻
ShortId = 'Short ID', | ||
ParticipantID = 'Participant ID' | ||
} | ||
|
||
export default class ParticipantListPage { |
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.
ParticipantListPage extends DSMPageBase
? because you created new abstract class DSMPageBase
.
export const setPatientParticipantGuid = async (page: Page) => { | ||
const [response] = await Promise.all([ | ||
page.waitForResponse((resp) => resp.url().includes('/participants') && resp.status() === 200) | ||
]); | ||
|
||
const participantResponse = response.url(); | ||
const urlArray = participantResponse.split('/'); | ||
user.patient.participantGuid = urlArray[6]; | ||
}; |
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.
Waiting and parsing a request in a separate function is going to make tests flaky because request could have been finished before this function starts to run. Best way to do this is, perform action and wait for response together a in Promise.all.
await Promise.all([
some_action(),
waitForResponse()
]);
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.
will do - thinking a bit more about this one - in terms of which action to use - will make the changes
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.
|
||
test.describe('Adult Self Enrollment', () => { | ||
test.describe.serial('Adult Self Enrollment', () => { |
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.
Correct!
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.
Will take a look at the test and fix it post haste
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.
initial investigation - seems to run well in my localhost - but for some reason the pt above is in the dev database but doesn't seem to exist in elastic search - looking more into it and will report back
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.
kibana seems to be slow - so it might be more than a couple of mins
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.
correction - I used test env when checking local run - that's why it's working for me - new RGP pts since about March 10th don't seem to be appearing in DSM and that's why the test failed - looking into it
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.
*don't seem to be appearing in DSM dev I mean
playwright-e2e/package-lock.json
Outdated
"eslint-plugin-prettier": "^4.2.1", | ||
"prettier": "^2.8.4", |
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 removed prettier and related plugin. Do you think we want to keep it?
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.
no - let's take it out (will do!) - it ended up getting put in while using vs code
remaining work: some feedback, comparison between copied family member and proband (DSM), checking messages display (DSS) and then should be end to end. Will also add blood rnaseq column check. Then most apart from kit upload for RGP should be automated with this PR. |
flakiness in checking contents of Participant Info notes after revisiting page seems to be mostly fixed
move out most dsm related tests to its own script since test run time was approaching 180 seconds which is temporarily extended while doing testing - test time for dsm related stuff is now 70 seconds - will likely combine a test or two - other tests are skipped so that a run focuses on only adult enrollment and dsm - also when child enrollment fails screenshot comparison the guid info seems lost somehow - will get rid of skips once kit upload and dashboard part is done
@@ -16,15 +16,17 @@ | |||
}, | |||
"devDependencies": { | |||
"@faker-js/faker": "^7.6.0", | |||
"@playwright/test": "^1.31.1", | |||
"@playwright/test": "^1.33.0", |
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.
Don't upgrade Playwright version in this PR because you would also need to update CI Playwright docker image version, update test screenshots on Dev, Test and docker container.
currently rgp tests check the following: dss enrollment survey -> dsm to check the pt is there -> check the rgp dynamic fields -> family member creation -> kit upload. Added method to get most recent playwright pt. Added stuff from other pr since merge both option wasn't seen.
the @proband annotation is temporary while cleaning up tests
forgot to make family enrollment tests serial, the first test now searches for the most recently created automated pt and the rests of the tests after that should consistently use whatever pt the first test found. Could use refactoring later. My additions seem to pass in docker container now.
Plan is to break this PR into smaller PRs - a bit on hold until this ticket is done: https://broadworkbench.atlassian.net/browse/PEPPER-692 (since it will fail consistently in dev otherwise - currently passes when ran in tests which has less/minimal housekeeping issues) |
handled by other smaller PRs - closing this one |
Extends the current RGP playwright test to check that the newly created participant can also be seen in DSM - might need a slight tweak - the most recent changes were done using DSM Test and there's a slight text difference when selecting the RGP study there (DSM Test has
Pepper RGP
, DSM Dev hasRGP
). But other than that should be running fine.Finishes up the last part of of this ticket: https://broadworkbench.atlassian.net/browse/PEPPER-587