Improve the stability of SAML integ test #1237

RyanL1997 · 2022-12-02T21:03:13Z

Signed-off-by: Ryan Liang [email protected]

Description

Fix the stability of SAML integ test

Issues Resolved

Related [Various Issues] SAML Integration Tests #1228

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

~~New functionality includes testing~~
~~New functionality has been documented~~
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

peternied

Could you provide more details as to why the driver.findElement(...) path didn't work out? It seems like if we can't verify this scenario from the UX something is wrong with the product

peternied · 2022-12-02T22:41:38Z

test/jest_integration/saml_auth.test.ts

@@ -326,12 +326,14 @@ 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(


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.

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.

@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.

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

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:

Go to the overview page

Sign in (Not as clear how this is actually done in the test...)

Select the global tenant, confirm

Sign out

Sign back in

Skip the popup-ux

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.

@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]:

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

This sounds like a product bug?

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.

@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

peternied · 2022-12-08T22:29:51Z

Any updates for this PR, or anything we can help with?

RyanL1997 · 2022-12-09T14:47:53Z

Any updates for this PR, or anything we can help with?

Hi @peternied I was working with @scrawfor99 for the windows support for CI. This one I may need the help from @anijain-Amazon . Since he is the author of the original SAML integ test.

Signed-off-by: Ryan Liang <[email protected]>

codecov-commenter · 2022-12-09T17:53:23Z

Codecov Report

Merging #1237 (34b1087) into main (5419567) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1237   +/-   ##
=======================================
  Coverage   71.78%   71.78%           
=======================================
  Files          88       88           
  Lines        2027     2027           
  Branches      269      269           
=======================================
  Hits         1455     1455           
  Misses        509      509           
  Partials       63       63

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

RyanL1997 · 2022-12-09T22:07:14Z

I updated this PR with a temporary fix/enhancement by adding another tenant switching and check if it is the scenario that the webdriver doesn't reload to update the tenant info according to local storage. This will do both of improving the stability and testing the actual UI elements.

peternied

SMART! I love that while something is busted, this will increase the reliable and remove the pressure while attempting to validate the scenario as well as we can.

DarshitChanpura · 2022-12-12T17:58:17Z

@RyanL1997 Looks like both CI checks failed due to same reason: jarHell. Can you look into it and let us know if you need any help?. I see that there is a tracking issue here: opensearch-project/OpenSearch#5526

cwperks

Nice workaround and for persisting through this issue! It looks like this is an issue with the headless browser and not with the actual code for switching to a tenant based on the value in local storage.

Signed-off-by: Ryan Liang <[email protected]> (cherry picked from commit 7ffbf09)

Signed-off-by: Ryan Liang <[email protected]> (cherry picked from commit 7ffbf09) Co-authored-by: Ryan Liang <[email protected]>

RyanL1997 requested a review from a team December 2, 2022 21:03

RyanL1997 mentioned this pull request Dec 2, 2022

[Various Issues] SAML Integration Tests #1228

Closed

3 tasks

RyanL1997 added the backport 2.x backport to 2.x branch label Dec 2, 2022

peternied requested changes Dec 2, 2022

View reviewed changes

RyanL1997 force-pushed the fix-saml-stability branch 2 times, most recently from d60060e to 920df4f Compare December 9, 2022 17:33

Enhance the stability of SAML integ test

6b31de1

Signed-off-by: Ryan Liang <[email protected]>

RyanL1997 force-pushed the fix-saml-stability branch from 920df4f to 6b31de1 Compare December 9, 2022 17:42

peternied approved these changes Dec 9, 2022

View reviewed changes

RyanL1997 changed the title ~~Fix the stability of SAML integ test~~ Improve the stability of SAML integ test Dec 9, 2022

Merge branch 'main' into fix-saml-stability

8b1eb45

cwperks approved these changes Dec 13, 2022

View reviewed changes

DarshitChanpura approved these changes Dec 13, 2022

View reviewed changes

RyanL1997 added 2 commits December 13, 2022 15:37

Merge branch 'main' into fix-saml-stability

1a7e3ed

Merge branch 'main' into fix-saml-stability

34b1087

RyanL1997 merged commit 7ffbf09 into opensearch-project:main Dec 14, 2022

opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 14, 2022

Enhance the stability of SAML integ test (#1237)

1fa8517

Signed-off-by: Ryan Liang <[email protected]> (cherry picked from commit 7ffbf09)

opensearch-trigger-bot bot mentioned this pull request Dec 14, 2022

[Backport 2.x] Improve the stability of SAML integ test #1272

Merged

DarshitChanpura pushed a commit that referenced this pull request Dec 15, 2022

Enhance the stability of SAML integ test (#1237) (#1272)

f728d33

Signed-off-by: Ryan Liang <[email protected]> (cherry picked from commit 7ffbf09) Co-authored-by: Ryan Liang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the stability of SAML integ test #1237

Improve the stability of SAML integ test #1237

RyanL1997 commented Dec 2, 2022

peternied left a comment

peternied Dec 2, 2022

RyanL1997 Dec 3, 2022 •

edited

Loading

cwperks Dec 5, 2022 •

edited

Loading

cwperks Dec 5, 2022

peternied Dec 5, 2022

RyanL1997 Dec 5, 2022

peternied Dec 5, 2022

RyanL1997 Dec 5, 2022

cwperks Dec 6, 2022

peternied commented Dec 8, 2022

RyanL1997 commented Dec 9, 2022

codecov-commenter commented Dec 9, 2022 •

edited

Loading

RyanL1997 commented Dec 9, 2022

peternied left a comment

DarshitChanpura commented Dec 12, 2022 •

edited

Loading

cwperks left a comment

Improve the stability of SAML integ test #1237

Improve the stability of SAML integ test #1237

Conversation

RyanL1997 commented Dec 2, 2022

Description

Category

Issues Resolved

Testing

Check List

peternied left a comment

Choose a reason for hiding this comment

peternied Dec 2, 2022

Choose a reason for hiding this comment

RyanL1997 Dec 3, 2022 • edited Loading

Choose a reason for hiding this comment

cwperks Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

cwperks Dec 5, 2022

Choose a reason for hiding this comment

peternied Dec 5, 2022

Choose a reason for hiding this comment

RyanL1997 Dec 5, 2022

Choose a reason for hiding this comment

peternied Dec 5, 2022

Choose a reason for hiding this comment

RyanL1997 Dec 5, 2022

Choose a reason for hiding this comment

cwperks Dec 6, 2022

Choose a reason for hiding this comment

peternied commented Dec 8, 2022

RyanL1997 commented Dec 9, 2022

codecov-commenter commented Dec 9, 2022 • edited Loading

Codecov Report

RyanL1997 commented Dec 9, 2022

peternied left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Dec 12, 2022 • edited Loading

cwperks left a comment

Choose a reason for hiding this comment

RyanL1997 Dec 3, 2022 •

edited

Loading

cwperks Dec 5, 2022 •

edited

Loading

codecov-commenter commented Dec 9, 2022 •

edited

Loading

DarshitChanpura commented Dec 12, 2022 •

edited

Loading