CDF Login using OTA #197

Vipinofficial11 · 2023-08-21T04:36:33Z

Changes for CDF login flow using OTA credentials.

rmstar · 2023-08-21T15:39:31Z

src/main/resources/connectionParameters.properties

 screenshotForAllSteps=false
+testAccountName=CloudDataFution Automation


nit: Fution->Fusion

I am not sure why, but it actually shows CloudDataFution Automation on the UI

src/main/java/io/cdap/e2e/pages/actions/CdfPipelineRunAction.java

itsankit-google · 2023-08-24T17:09:45Z

src/main/java/io/cdap/e2e/pages/actions/CdfSignInActions.java

+import static io.cdap.e2e.utils.ConstantsUtil.CDFPASSWORD;
+import static io.cdap.e2e.utils.ConstantsUtil.CDFUSERNAME;


unless there is a specific usecase, we should not go for static imports, it makes the code unreadable.

Please use ConstantsUtil.CDFPASSWORD and so on wherever required.

@Vipinofficial11 can you address comment so that we can get to a closure sooner.

updated [36bc9cf]

itsankit-google · 2023-08-28T05:45:36Z

src/main/java/io/cdap/e2e/pages/locators/CdfSignInLocator.java

+        return By.xpath("//span[contains(text(),'Continue')]");
+    }
+
+    public static By locatePluginNameInList(String pluginName, String  pluginGroupName) {


This locator is already present in CdfStudioLocators, can we reuse it??

The Locator present in CdfStudioLocators has return type as WebElement while this one has a return type of By because the use case for both is a bit different.

The locatePluginNameInList in CdfStudioLocators is being used to check if we are able to find a plugin in the plugin list. (Here we are using this with WaitHelper.waitForElementToBeDisplayed)

The locatePluginNameInList in CdfSingInLocators is being used in sort of a retry mechanism where we are checking if after clicking on the Continue button we are able to see the studio page or a plugin in the plugin list. (Here we are using this with WaitHelper.waitForElementToBeOptionallyDisplayed)

The locator surely should be moved to CdfStudioLocators but I don't see a way of reusing the one whose return type is WebElement.

What do you think, is there any way using which we can reuse the locator and achieve both use cases.

Commented: #197 (comment)

itsankit-google · 2023-08-28T05:47:53Z

src/main/java/io/cdap/e2e/pages/actions/CdfPipelineRunAction.java

+      Boolean.parseBoolean(SeleniumHelper.readParameters(ConstantsUtil.TESTONHDF))) {
+      do {
+        PageHelper.refreshCurrentPage();
+        SeleniumDriver.getWaitDriver(ConstantsUtil.PAGE_LOAD_TIMEOUT_SECONDS);


A page refresh in every 50seconds might be too frequent, it won't change the pipeline status drastically, I would suggest adding a sleep of atleast 2 mins.

Updated the sleep time to 2mins, re-using PIPELINE_DEPLOY_TIMEOUT_SECONDS as it's value is 120, but the constant name can create some confusion here.

Should we create a new constant with same value named as PIPELINE_REFRESH_TIMEOUT_SECONDS here ?

(44be5d5#diff-cfa45894e4592a5d6ed32f485ed2594ae01b9a3b154e60820a599164e0b52e80R136)

Made some changes to this function PTAL

itsankit-google · 2023-08-28T15:50:23Z

src/main/java/io/cdap/e2e/pages/actions/CdfSignInActions.java

+        ElementHelper.clickIfDisplayed(cdfSignInLocator.clickOnContinueButton(), ConstantsUtil.MEDIUM_TIMEOUT_SECONDS,
+          cdfSignInLocator.locatePluginNameInList(ConstantsUtil.FIRST_PLUGIN_IN_LIST, "Source"));
+
+        ElementHelper.clickIfDisplayed(cdfSignInLocator.clickOnAllowButton(), ConstantsUtil.MEDIUM_TIMEOUT_SECONDS,
+          cdfSignInLocator.locatePluginNameInList(ConstantsUtil.FIRST_PLUGIN_IN_LIST, "Source"));


Why do we want to combine two operations here, why can't we do something like this?

Click on element1 if displayed

Click on element2 if displayed

Wait until plugin element is displayed.

The main reason behind creating clickIfDisplayed was to implement a retry if the element is not displayed on the first attempt.

The reason to combine 2 operations was related to this retry only, the idea was that if once we clicked on the desired element we will validate if the next expected element (3rd parameter) is displayed if we don't see the next element we try to click on the desired element again.

Will this retry be implemented in future? I don't see it being retried in this PR or am I missing something?

The Retry logic is actually implemented in clickIfDisplayed

clickIfDisplayed(locatorToClick, timeOutInSeconds); // If the next webElement is not displayed we will click on the desired element again. if (!isElementDisplayed(locatorToWaitFor, timeOutInSeconds)) { logger.info("Retry : Click on " + locatorToClick); clickIfDisplayed(locatorToClick);

(https://github.com/cdapio/cdap-e2e-tests/pull/197/files#diff-1b63d2aab0be21d4b99280309e329d962acb6a5c2bb82e6075c8da4ce3b6569eR134)

why is the retry needed? The click should result in the next element within timeout. If we retry, we might initiate the same operation twice.

I agree that this can result in same operation twice, the main purpose of adding the refresh here was to avoid any flakiness in case of CDF login flow because I observed few failed executions while executing the flow. I am not completely sure why but sometimes the click was getting intercepted. (We were waiting for elementToBeClickable still there were some failed executions due to this)

We can remove this refresh and use a simple click with a timeout, I will just have to make sure that there is no flakiness while using a simple click on Cloud Build.

what do you guys suggest ? (cc : @itsankit-google)

samdgupi · 2023-09-01T13:54:11Z

src/main/java/io/cdap/e2e/pages/actions/CdfSignInActions.java

+        ElementHelper.sendKeys(cdfSignInLocator.cdfPassword, SeleniumHelper.readParameters(ConstantsUtil.CDFPASSWORD));
+        ElementHelper.clickOnElement(cdfSignInLocator.nextButton);
+
+        // Adding optional operations here.


what is this comment referring to ?

Based on the observations in case of CDF test execution from local we encountered an Allow Button and in case of CDF test execution from Cloud Build we encountered Continue Button so to handle this scenario the button clicks were optional.

That's not the case now I see same Continue button on both executions now, so removed the Allow button click and comment as well.

samdgupi · 2023-09-01T13:55:17Z

src/main/java/io/cdap/e2e/pages/actions/CdfSignInActions.java

+        are using 2 confirmation buttons here.
+        */
+
+        String testAccountName = SeleniumHelper.readParameters("testAccountName");


Please define constant for "testAccountName"

Created a constant CDF_TEST_ACCOUNT_NAME for this.

(44be5d5#diff-d1ebf05f182d1decee9527cecadb7ea940ba7cc8dddc7519c80dd3e4b64db94fR40)

samdgupi · 2023-09-01T14:02:31Z

src/main/java/io/cdap/e2e/utils/ConstantsUtil.java

@@ -93,4 +96,9 @@ public class ConstantsUtil {
   * PIPELINE_RUN_TIMEOUT_SECONDS: To be used as a timeout for Pipeline Runs
   */
  public static final int PIPELINE_RUN_TIMEOUT_SECONDS = 900;
+
+  /**
+   * MEDIUM_TIMEOUT_SECONDS: To be used as a wait for retry


Can you please make the comment and name specific.
This is used to wait for elemnt to be displayed right ?

The small timeout works fine here, so removed this Timeout.

samdgupi · 2023-09-01T14:03:41Z

src/main/java/io/cdap/e2e/utils/ConstantsUtil.java

+  /**
+   * MEDIUM_TIMEOUT_SECONDS: To be used as a wait for retry
+   */
+  public static final int MEDIUM_TIMEOUT_SECONDS = 10;


Looking at the usage would not SMALL_TIMEOUT_SECONDS work ?

It works on local execution, used SMALL_TIMEOUT_SECONDS instead of medium everywhere for now.

Note : There is a possibility that due to some external delay in the Login Flow a small timeout may cause some flakiness.

(44be5d5#diff-653a26322c5148b982b3df96f0640a19488ea600b8501ce2695a5218dda8be2bR45)

samdgupi · 2023-09-01T14:36:09Z

src/main/java/io/cdap/e2e/pages/actions/CdfSignInActions.java

+        ElementHelper.clickIfDisplayed(cdfSignInLocator.clickOnContinueButton(), ConstantsUtil.MEDIUM_TIMEOUT_SECONDS,
+          cdfSignInLocator.locatePluginNameInList(ConstantsUtil.FIRST_PLUGIN_IN_LIST, "Source"));
+
+        ElementHelper.clickIfDisplayed(cdfSignInLocator.clickOnAllowButton(), ConstantsUtil.MEDIUM_TIMEOUT_SECONDS,
+          cdfSignInLocator.locatePluginNameInList(ConstantsUtil.FIRST_PLUGIN_IN_LIST, "Source"));


why is the retry needed? The click should result in the next element within timeout. If we retry, we might initiate the same operation twice.

samdgupi · 2023-09-01T14:41:08Z

src/main/java/io/cdap/e2e/pages/actions/CdfPipelineRunAction.java

+        SeleniumDriver.getWaitDriver(ConstantsUtil.PAGE_LOAD_TIMEOUT_SECONDS);
+      } while (isStarting() || isRunning() || isProvisioning());
+    }
+
    SeleniumDriver.getWaitDriver(ConstantsUtil.PIPELINE_RUN_TIMEOUT_SECONDS).until(ExpectedConditions.or(
      ExpectedConditions.visibilityOf(CdfPipelineRunLocators.succeededStatus),


This wait seems unneeded based on the above retry which will only stop when the pipeline is in a terminal state

Agree, The purpose of this wait here was if the pipeline is in Running status for more than 15 minutes we should mark that pipeline as failed to avoid a very long Build execution time.

Now I realised that after the above refresh logic, the do-while loop keep on refreshing the page until the pipeline is in running state which is a problem what if pipeline takes 50-60 mins.

So added a logic in the do-while to break the loop if the pipeline is in running state for more that 900second or 15 mins also changed the PIPELINE_RUN_TIMEOUT_SECONDS = 900second to IMPLICIT_TIMEOUT_SECONDS = 30second to check if Failed || Success || Stop status is seen.

cc: @itsankit-google

(44be5d5#diff-cfa45894e4592a5d6ed32f485ed2594ae01b9a3b154e60820a599164e0b52e80R122)

samdgupi · 2023-09-05T05:50:14Z

src/main/java/io/cdap/e2e/pages/actions/CdfPipelineRunAction.java

+      // Adding pipelineExecutionTimeFlag to break the loop if pipeline status is Running state for more than
+      // 900 seconds.
+      do {
+        pipelineExecutionTimeFlag += 120;


Can we please define a retry function which takes a retry delay, max retry delay and max retrycount. This can be used both here and in clickIfDisplayed

Also please make the retry delays as constants

Added the retry function and re-used it for both pipeline refresh and button click. (5c54894)

Vipinofficial11 · 2023-10-03T05:11:19Z

Current Build Failures are not related to these changes. Team is working on fixing these failures.

samdgupi · 2023-10-03T05:35:18Z

src/main/java/io/cdap/e2e/pages/actions/CdfPipelineRunAction.java

+        }
+      );
+
+    SeleniumDriver.getWaitDriver(ConstantsUtil.IMPLICIT_TIMEOUT_SECONDS).until(ExpectedConditions.or(


This looks similar to waitTillPipelineRunCompletes. Please reuse.

Re-used (66e7a80)

Vipinofficial11 · 2023-10-05T15:45:25Z

Merging the PR as it is approved by 2 reviewers (Tested these changes on CDAP as well as on CDF setup)

Vipinofficial11 force-pushed the testOtaLogin branch 2 times, most recently from 3bcdf98 to cb3c7e2 Compare August 21, 2023 08:13

sau42shri requested review from rmstar and samdgupi August 21, 2023 15:38

rmstar reviewed Aug 21, 2023

View reviewed changes

rmstar approved these changes Aug 21, 2023

View reviewed changes

itsankit-google reviewed Aug 24, 2023

View reviewed changes

itsankit-google reviewed Aug 28, 2023

View reviewed changes

itsankit-google added the build label Aug 29, 2023

samdgupi reviewed Sep 1, 2023

View reviewed changes

samdgupi reviewed Sep 5, 2023

View reviewed changes

Vipinofficial11 force-pushed the testOtaLogin branch 3 times, most recently from 0c38fc6 to 37bbeee Compare September 26, 2023 08:13

Vipinofficial11 force-pushed the testOtaLogin branch from 37bbeee to 5c54894 Compare October 3, 2023 03:28

samdgupi reviewed Oct 3, 2023

View reviewed changes

samdgupi approved these changes Oct 3, 2023

View reviewed changes

CDF Login

8861cb5

Vipinofficial11 force-pushed the testOtaLogin branch from 498f799 to 8861cb5 Compare October 5, 2023 13:03

Vipinofficial11 merged commit 5fced8e into cdapio:develop Oct 5, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDF Login using OTA #197

CDF Login using OTA #197

Vipinofficial11 commented Aug 21, 2023

rmstar Aug 21, 2023

Vipinofficial11 Aug 21, 2023

itsankit-google Aug 24, 2023

sau42shri Aug 25, 2023

Vipinofficial11 Aug 28, 2023 •

edited

Loading

itsankit-google Aug 28, 2023

Vipinofficial11 Aug 28, 2023 •

edited

Loading

itsankit-google Aug 28, 2023

itsankit-google Aug 28, 2023

Vipinofficial11 Sep 4, 2023 •

edited

Loading

itsankit-google Aug 28, 2023

Vipinofficial11 Aug 29, 2023

itsankit-google Aug 31, 2023

Vipinofficial11 Sep 1, 2023

samdgupi Sep 1, 2023

Vipinofficial11 Sep 4, 2023 •

edited

Loading

samdgupi Sep 1, 2023

Vipinofficial11 Sep 4, 2023

samdgupi Sep 1, 2023

Vipinofficial11 Sep 4, 2023

samdgupi Sep 1, 2023

Vipinofficial11 Sep 4, 2023

samdgupi Sep 1, 2023

Vipinofficial11 Sep 4, 2023 •

edited

Loading

samdgupi Sep 1, 2023

samdgupi Sep 1, 2023

Vipinofficial11 Sep 4, 2023 •

edited

Loading

samdgupi Sep 5, 2023

samdgupi Sep 5, 2023

Vipinofficial11 Oct 3, 2023

Vipinofficial11 commented Oct 3, 2023

samdgupi Oct 3, 2023

Vipinofficial11 Oct 3, 2023

Vipinofficial11 commented Oct 5, 2023

		screenshotForAllSteps=false
		testAccountName=CloudDataFution Automation

		import static io.cdap.e2e.utils.ConstantsUtil.CDFPASSWORD;
		import static io.cdap.e2e.utils.ConstantsUtil.CDFUSERNAME;

CDF Login using OTA #197

CDF Login using OTA #197

Conversation

Vipinofficial11 commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipinofficial11 commented Oct 5, 2023

Vipinofficial11 Aug 28, 2023 •

edited

Loading

Vipinofficial11 Aug 28, 2023 •

edited

Loading

Vipinofficial11 Sep 4, 2023 •

edited

Loading

Vipinofficial11 Sep 4, 2023 •

edited

Loading

Vipinofficial11 Sep 4, 2023 •

edited

Loading

Vipinofficial11 Sep 4, 2023 •

edited

Loading