Skip to content
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.

Sign up for GitHub

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

Improve the stability of SAML integ test #1237

Merged
merged 4 commits into from
Dec 14, 2022
Merged

Improve the stability of SAML integ test #1237

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b31de1
Enhance the stability of SAML integ test
RyanL1997 Dec 9, 2022
8b1eb45
Merge branch 'main' into fix-saml-stability
RyanL1997 Dec 10, 2022
1a7e3ed
Merge branch 'main' into fix-saml-stability
RyanL1997 Dec 13, 2022
34b1087
Merge branch 'main' into fix-saml-stability
RyanL1997 Dec 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion test/jest_integration/saml_auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ describe('start OpenSearch Dashboards server', () => {
const skipWelcomeBtnXPath = '//button[@data-test-subj="skipWelcomeScreen"]';
const tenantNameLabelXPath = '//*[@id="tenantName"]';
const pageTitleXPath = '//*[@id="osdOverviewPageHeader__title"]';
const tenantSwitchBtnXPath = '//button[@data-test-subj="switch-tenants"]';
// Browser Settings
const browser = 'firefox';
const options = new Options().headless();
Expand Down Expand Up @@ -327,11 +328,42 @@ describe('start OpenSearch Dashboards server', () => {
await driver.wait(until.elementsLocated(By.xpath(tenantNameLabelXPath)), 10000);

const tenantName = await driver.findElement(By.xpath(tenantNameLabelXPath)).getText();
const localStorageItem = await driver.executeScript(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Know why the text wasn't loading? These test are suppose to make sure the UX is rendering the correct content, and this change effectively works around inspecting the UI - this is a major hit to the functionality of the test to use the localstorage state.

Copy link
Collaborator Author

@RyanL1997 RyanL1997 Dec 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @peternied, thanks for the review! I totally agree that we should test out the actual UI elements. However, according to the experiments @cwperks and I did on my local machine, it is not about the loading of that UI elements. It is something about the selenium webdriver, and I have also tried out different webdrivers such as chromedriver and they all have a very high chance for not go through the second window.reload() for updating the saved tenant according to the localstorage key. So there is nothing we can do about the webdriver for now :(

However, I'm willing to figure out a work around as an another approach if it is necessary.

Copy link
Member

@cwperks cwperks Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RyanL1997 I was just looking at the requests that happen after login and I believe what we can do is wait for a response from http://<osd_baseurl>/api/v1/multitenancy/tenant before clicking on the user dropdown and viewing the tenant info. This endpoint is called after login to get the current tenant info from what I can see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With playwright its possible to do a page.waitForResponse(urlOrPredicate... , but I'm not sure if selenium has something similar.

https://playwright.dev/docs/api/class-page#page-wait-for-response

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can dig into this a little more to see what the test should do verses what is hard to do, Quickly reading the test:

  1. Go to the overview page
  2. Sign in (Not as clear how this is actually done in the test...)
  3. Select the global tenant, confirm
  4. Sign out
  5. Sign back in
  6. Skip the popup-ux
  7. Wait until the tenant label appears and show Global

Seems like knowing we are on the global tenant is a critical part of the test

Can we find an element that is blank until the tenant query has resolved? We could add an element with an ID that would be easy to find and wait for it to disappear when the tenant has loaded if that is part of the problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peternied Thanks for this info. The problem we are having right now is not about the loading of the UI. We are can load and locate this UI. However, it just doesn't switch to the expected tenant (It sometimes stays as "private" [1]). For now, I am trying to implement a retry for this scenario, and I am also taking a look into @cwperks 's suggestion to see how much effort will it take us to switch to another test frame work.

Reference:
[1]:
Screenshot 2022-12-05 at 1 47 07 PM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it just doesn't switch to the expected tenant (It sometimes stays as "private" [1])

This sounds like a product bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don think so, because, based on the experiment I did so far, it wont happen to the regular browser, and it is just the webdriver we are using got a high chance to not reload the window for updating tenant information according to the local storage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RyanL1997 Another thought, maybe its possible to use until.elementTextIs(element, text)?

https://www.selenium.dev/selenium/docs/api/javascript/module/selenium-webdriver/lib/until.html

`return window.localStorage.getItem("opendistro::security::tenant::saved")`
);

// Retry previous steps one more time if the webdriver doens't reload as expected
if (tenantName === 'Private' && localStorageItem === '""') {
await driver.wait(until.elementsLocated(By.xpath(tenantSwitchBtnXPath)), 10000);
await driver.findElement(By.xpath(tenantSwitchBtnXPath)).click();

await driver.executeScript('arguments[0].scrollIntoView(true);', radio);
await driver.executeScript('arguments[0].click();', radio);
await driver.wait(until.elementIsSelected(radio));

await driver.findElement(By.xpath('//button[@data-test-subj="confirm"]')).click();

await driver.wait(until.elementsLocated(By.xpath(userIconBtnXPath)), 10000);
await driver.findElement(By.xpath(userIconBtnXPath)).click();
await driver.findElement(By.xpath('//*[@data-test-subj="log-out-1"]')).click();

await driver.wait(until.elementsLocated(By.xpath(signInBtnXPath)), 10000);
await driver.findElement(By.xpath(signInBtnXPath)).click();

await driver.wait(until.elementsLocated(By.xpath(userIconBtnXPath)), 10000);
await driver.findElement(By.xpath(userIconBtnXPath)).click();
await driver.wait(until.elementsLocated(By.xpath(tenantNameLabelXPath)), 10000);

const newtenantName = await driver.findElement(By.xpath(tenantNameLabelXPath)).getText();
expect(newtenantName).toEqual('Global');
} else {
expect(localStorageItem).toEqual('""');
expect(tenantName).toEqual('Global');
}
await driver.manage().deleteAllCookies();
await driver.quit();

expect(tenantName).toEqual('Global');
expect(localStorageItem).toEqual('""');
});
});

Expand Down