test: add critical flow test for on-prem login redirect [WPB-25319]#21407
test: add critical flow test for on-prem login redirect [WPB-25319]#21407JacquelineLehner wants to merge 1 commit into
Conversation
Add a critical flow E2E test to verify the on-premise login redirect behaviour when a user enters an email associated with a claimed domain on the SSO page. Changes included: - Add `onPremLoginRedirect-TC-8757.spec.ts` to test the redirect flow and verify the custom backend dialog. - Implement `CustomBackendPage` POM. - Update `brig.repository.e2e.ts` with `claimDomain` and `deleteDomain` API helpers to handle test setup and guaranteed teardown. - Use FakerJS to ensure domain uniqueness per test run to prevent collisions. Refs: WPB-25319
JacquelineLehner
requested review from
arjita-mitra,
e-maad,
otto-the-bot,
screendriver,
thisisamir98 and
zskhan
as code owners
|
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
|
| ); | ||
| } | ||
|
|
||
| public async deleteDomain(domain: string) { |
There was a problem hiding this comment.
🎨 Maybe call it deleteDomainClaim() or something in the sense of "unclaim" domain just so it's clear that it's not a domain which is being deleted here 🙈
| config_url: 'https://nginz-https.anta.wire.link/deeplink.json', | ||
| webapp_url: 'https://webapp.anta.wire.link', |
There was a problem hiding this comment.
Could we maybe pass these config options as parameters? It feels a bit wrong to have them hardcoded and hidden in here. Maybe we could even re-use the env vars which will be added for the federation tests here, since they also point to Anta?
| await test.step('Open login page and enter email with claimed domain', async () => { | ||
| await pageManager.openSSOPage(); | ||
| await pages.singleSignOn().enterEmailOnSSOPage(email); | ||
| await pages.singleSignOn().ssoSignInButton.click(); |
There was a problem hiding this comment.
🔧 I don't think you need to click the sign in button yourself. When testing it it automatically shows the dialog after entering it. It might still work if the button is clicked before the dialog opens automatically, but let's try to avoid undefined timing specific behavior :)
|
|
||
| try { | ||
| await test.step('Claim domain and configure on-prem redirect', async () => { | ||
| await api.brig.claimDomain(domain); |
There was a problem hiding this comment.
The claiming of the domain is specified as precondition for the test, so no need to do it inside it. I'd suggest to put it in a beforeEach hook outside the test, this way you also don't need the manual try-finally block as you can just do the deletion inside a afterEach hook then :)
| const urlPattern = new RegExp(ON_PREM_WEBAPP_URL.replace(/\./g, '\\.')); | ||
|
|
||
| await Promise.all([ | ||
| page.waitForURL(urlPattern, { | ||
| timeout: 20_000, | ||
| }), | ||
| pages.customBackend().connectButton.click(), | ||
| ]); | ||
|
|
||
| await expect(page).toHaveURL(urlPattern); |
There was a problem hiding this comment.
Why so complicated? In the end all you're doing is clicking a button and waiting for the URL to change, there's no need to start listening for the URL change before the button is clicked. Also the Pattern for matching the URL can be simplified by using String.raw to escape it automatically (no need to maintain the escape regex ourself 😉)
| const urlPattern = new RegExp(ON_PREM_WEBAPP_URL.replace(/\./g, '\\.')); | |
| await Promise.all([ | |
| page.waitForURL(urlPattern, { | |
| timeout: 20_000, | |
| }), | |
| pages.customBackend().connectButton.click(), | |
| ]); | |
| await expect(page).toHaveURL(urlPattern); | |
| await page.getByRole('button', {name: 'Connect'}).click(); | |
| await expect(page).toHaveURL(new RegExp(String.raw`${ON_PREM_WEBAPP_URL}`), {timeout: 20_000}); |
| await expect(pages.customBackend().title).toBeVisible(); | ||
| await expect(pages.customBackend().redirectWarningText).toBeVisible(); | ||
| await expect(pages.customBackend().adminInfoText).toBeVisible(); |
There was a problem hiding this comment.
🔧 Let's try not to be overly specific here. Testing the exact texts shown on the page isn't a good practice as they easily change which would break the test for no reason.
Looking into the test description I don't think it's necessary to assert the redirectWarningText and adminInfoText. I'd say it should be sufficient to just check the page contains the headline and optionally the domain of the backend the user will be redirected to:
| await expect(pages.customBackend().title).toBeVisible(); | |
| await expect(pages.customBackend().redirectWarningText).toBeVisible(); | |
| await expect(pages.customBackend().adminInfoText).toBeVisible(); | |
| await expect(page.getByText("Connect to your organization's backend?")).toBeVisible(); | |
| await expect(page.getByText(ON_PREM_WEBAPP_URL)).toBeVisible(); |
This way the test is less likely to break for unrelated reasons which means less maintenance effort ;)
Also notice ho I inlined the locator for the title. This locator will most likely be only used once in here as it is very specific. I think we don't need the dedicated POM for now. Also having the title in the test itself makes it easier to read :)
2 participants
This PR introduces a critical flow E2E test (TC-8757) to verify the on-premise login redirect behavior. It ensures that users entering an email associated with a claimed domain on the SSO page are correctly prompted and redirected to the correct backend.
Refs: WPB-25319