Sap odp by sumitnigamcs · Pull Request #10 · data-integrations/sap

sumitnigamcs · 2021-11-24T10:59:11Z

Please review

itsankit-google · 2021-11-25T17:44:56Z

src/e2e-test/java/io/cdap/plugin/odp/locators/ODPLocators.java

+ */
+public class ODPLocators {
+
+  @FindBy(how = How.XPATH, using = "//*[contains(text(),'SAP ODP')]")


should use something like @FindBy(how = How.XPATH, using = "//*[@data-cy=\"plugin-SapOdp-batchsource\"]") here

itsankit-google · 2021-11-25T17:46:44Z

src/e2e-test/java/io/cdap/plugin/odp/locators/ODPLocators.java

+  @FindBy(how = How.XPATH, using = "//*[contains(text(),'SAP ODP')]")
+  public static WebElement sapODPSource;
+
+  @FindBy(how = How.XPATH, using = "//*[contains(text(),'-SNAPSHOT')]")


can this locator be made more specific so that it is used to locate properties of SAP ODP plugin specifically?because there can be other plugins as well which can match this.

itsankit-google · 2021-11-25T17:52:42Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/DesginTimeODP.java

+  @When("User has selected Sap client macro to configure")
+  public void userHasSelectedSapClientMacroToConfigure() {
+    ODPLocators.macros.get(0).click();
+    ODPLocators.sapClient.sendKeys(Keys.chord(Keys.CONTROL, A));


concerned if this operation is OS specific because in MAC we use CMD+A to perform this operation?
If the use case is to clear the input text and add value can we use SeleniumHelper.replaceElementValue() for this? @rmstar what do you think?

itsankit-google · 2021-11-25T17:52:59Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/DesginTimeODP.java

+  @When("User has selected Sap language macro to configure")
+  public void userHasSelectedSapLagMacroToConfigure() {
+    ODPLocators.macros.get(1).click();
+    ODPLocators.language.sendKeys(Keys.chord(Keys.CONTROL, A));


similar comment as above in the whole PR wherever (Keys.CONTROL, A) is used.

itsankit-google · 2021-11-25T17:53:41Z

src/e2e-test/java/io/cdap/plugin/odp/locators/ODPLocators.java

+  public static WebElement validateButton;
+
+  @FindBy(how = How.XPATH, using = "//*[@data-cy='plugin-validation-success-msg']")
+  public static WebElement successMessage;


successMessage -> pluginValidationSuccessMsg

itsankit-google · 2021-11-25T18:06:34Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/DesginTimeODP.java

+
+  @When("User has selected Sap server as host macro to configure")
+  public void userHasSelectedSapServerAsHostMacroToConfigure() {
+    ODPLocators.macros.get(2).click();


Is there any better way to handle this? This makes the test brittle. In future, if the plugin configs order is changed then whole test will break! @rmstar What do you think?

@itsankit-google I dont think so we can handle it any other way as these macros are in a single class only the index is the difference. If order changes then we need to again change the order of identification

yeah seems brittle.. what happens if a new config param is added? would that affect the index of other macros?
would be good if we can find a better way to handle it.

got the solution for every macro element we can capture an object path will remain constant- //*[@data-cy="jco.client.ashost"]//child::button//span//span//strong only the data-cy value will be changed. @sumitnigamcs please capture all the macros seprately and call via name

itsankit-google · 2021-11-25T18:07:14Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/DesginTimeODP.java

+
+  @When("User has selected System Number macro to configure")
+  public void userHasSelectedSystemNumberMacroToConfigure() {
+    ODPLocators.macros.get(3).click();


similar comment as above in the whole PR wherever macros are used.

itsankit-google · 2021-11-25T18:13:08Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/RuntimeODP.java

+    InputStream input;
+    Properties connection = new SAPProperties();
+    input = new FileInputStream("src/e2e-test/resources/Google_SAP_Connection.properties");
+    prop.load(input);


can we load property file once instead on every call?

itsankit-google · 2021-11-25T18:31:51Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/RuntimeODP.java

+  @Then("Get Count of no of records transferred from ODP to BigQuery in {string}")
+  public void getCountOfNoOfRecordsTransferredFromODPToBigQueryIn(String table) throws
+    IOException, InterruptedException {
+    countRecords = gcpClient.countBqQuery(CDAPUtils.getPluginProp(table));


Here, we should also verify the metric in the UI, no of records out from the source to the no of records in sink.

will fix this in CDAP-18634

tivv · 2021-12-01T18:35:26Z

pom.xml

+            <version>3.0.0-M5</version>
+            <configuration>
+              <includes>
+                <include>TestRunner.java, TestRunnerMand.java, TestRunnerOpt.java</include>


Use pattern instead of listing all classes

@tivv since we have to maintain the execution order of the TestRunners, we had to add the runner filenames like this. We have to make sure TestRunnerOpt runs in the end, else it will impact TestRunnerMand

Why do we need an execution order? It sounds like an issue. All tests must be independent. One should be able to run any test separately or tests in any sequence. That's basic principle.

@tivv For config driven scenarios, wherein we are implementing cucumberJvm plugin, we are loading optional properties on the run, once they are loaded, the scenarios with only mandatory fields will not run. So mandatory field scenario should run first, then plugin loading the optional properties should execute followed by optional field scenarios. Suggest if I am not able to put forward my point, or if I am missing something to make the scenarios more independent

Sure, could you please clarify in more details. Overall approach is that tests must be self-contained and do any necessary clean-up as needed. Why is it not possible here?

done, thanks

rmstar · 2021-11-30T04:19:32Z

pom.xml

      <scope>provided</scope>
    </dependency>
+    <dependency>
+      <groupId>com.google.guava</groupId>


why are we adding a guava dependency here?

I tried adding inside e2e-tests profile but it was not getting resolved there.

What are the issues? The way it's not it's not going to fly. Guava versioning is pretty tricky, so we must not add it to production code unless needed by production code

Hi @tivv I have removed guava from main dependency section, moved it to profile's dependency management, along with adding it to classpath via profile's maven-dependency-plugin. Please confirm if this is fine

rmstar · 2021-12-02T05:57:44Z

pom.xml

        </executions>
      </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>


why are these added outside e2e-tests profile?

removed, these were rather duplicate

rmstar · 2021-12-02T05:58:48Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/DesginTimeODP.java

+/**
+ * DesginTimeODP.
+ */
+public class DesginTimeODP {


nit: spelling

rmstar · 2021-12-02T06:01:42Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/DesginTimeODP.java

+  @When("User has selected Sap msServ macro to configure")
+  public void userHasSelectedSapMsServMacroToConfigure() {
+    ODPLocators.macros.get(4).click();
+    SeleniumHelper.replaceElementValue(ODPLocators.portNumber, "${portN0}");


Why is it N0 (zero instead of 'O')

rmstar · 2021-12-02T06:02:00Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/DesginTimeODP.java

+  @When("User has selected Sap msHost macro to configure")
+  public void userHasSelectedSapMsHostMacroToConfigure() {
+    ODPLocators.macros.get(2).click();
+    SeleniumHelper.replaceElementValue(ODPLocators.msHost, "${msHOst}");


${msHost} ?

Thats a macro variable which we have taken.

rmstar · 2021-12-02T06:14:16Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/RuntimeODP.java

+      JsonNode response = sapAdapterImpl.executeRFC(objectNode.toString(), opProps,
+                                                    StringUtils.EMPTY, StringUtils.EMPTY);
+    } catch (Exception e) {
+      throw SystemException.throwException(e.getMessage(), e);


why you do need to do this?

to abort the scenario in case record is not created

rmstar · 2021-12-02T06:16:09Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/RuntimeODP.java

+      (table) + "` WHERE " +
+      field.toUpperCase();
+    switch (datasource) {
+      case "0MATERIAL_ATTR":


use constants for datasources and actions if possible

rmstar · 2021-12-02T06:17:30Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/RuntimeODP.java

+  static String deltaLog;
+  static String rawLog;
+  static PrintWriter out;
+  private List<String> numList = new ArrayList<>();


what is stored in numList? maybe use a more descriptive name?

rmstar · 2021-12-02T06:21:12Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/RuntimeODP.java

+      field.toUpperCase();
+    switch (datasource) {
+      case "0MATERIAL_ATTR":
+        for (i = 0; i < numList.size(); i++) {


for (String thing : numList) {
// do stuff with thing
}

rmstar · 2021-12-02T06:22:20Z

src/e2e-test/java/io/cdap/plugin/odp/stepsdesign/RuntimeODP.java

+        BeforeActions.scenario.write("Table Deleted Successfully");
+      }
+    } catch (Exception e) {
+      BeforeActions.scenario.write(e.toString());


should we fail the test if an exception was thrown?

No we should not fail as if the table is not found in the catch block it will return table not found and we have written the same in the report.

tivv

What are the jars added? It's usually an antipattern as we must be very careful on versioning / licensing / availability.

sumitnigamcs · 2021-12-07T12:11:39Z

What are the jars added? It's usually an antipattern as we must be very careful on versioning / licensing / availability.

Hi @tivv , we have added the jar com.google.adapter.cs-0.0.1.jar, which has JCO connectors for record creation deletion and updation. Please suggest if there is any other way to use the jar through artifactory or something

tivv · 2021-12-09T22:43:06Z

POMs look good. What's about the jars? I don't think we can include them like this

sumitnigamcs · 2021-12-10T09:47:26Z

POMs look good. What's about the jars? I don't think we can include them like this.
@tivv working on the jars removal now.

sumitnigamcs · 2021-12-24T03:52:18Z

POMs look good. What's about the jars? I don't think we can include them like this

Removed the jars, and added github actions details to README, and a mention about it in pom. Please have a look
@tivv @rmstar @itsankit-google

sumitnigamcs · 2022-01-05T07:20:26Z

@tivv @rmstar @itsankit-google all review comments addressed. Please review, mainly on the jars removal.

sahilkapoor301992 and others added 8 commits November 20, 2021 18:32

ODP Changes

5f10bfa

Fix Code

b4ef000

Jar Password commented

6e14311

updated RFM changes

2c8cdb2

refactoring

91f5e84

temp file removed

c90e335

checkstyle

1412f8d

review comments adjusted

c3cf646

itsankit-google reviewed Nov 25, 2021

View reviewed changes

ksahilCF and others added 4 commits November 27, 2021 10:02

latest changes RFM

b5a57e0

review comments

42a373a

merge conflicts

05647e6

locators modified

f437120

itsankit-google requested a review from rmstar November 30, 2021 06:16

tivv reviewed Dec 1, 2021

View reviewed changes

rmstar reviewed Dec 2, 2021

View reviewed changes

review comments

e102746

tivv suggested changes Dec 3, 2021

View reviewed changes

sumitnigamcs added 4 commits December 4, 2021 05:32

macro changes

807aa15

license

c354cee

checkstyle

103a803

runner unsequencing

f5fe530

sumitnigamcs added 3 commits December 7, 2021 08:04

bug fix

03a47db

secure scenario

63b8968

rename prop

1601d9c

sumitnigamcs added 2 commits December 7, 2021 12:19

duplicate plugins

b3a64c4

guava moved to test profile

d5d64a2

jars removed and github action details added to readme

f623984

itsankit-google mentioned this pull request Jan 18, 2022

Util methods cdapio/cdap-e2e-tests#52

Merged

Conversation

sumitnigamcs commented Nov 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsankit-google Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

itsankit-google Nov 25, 2021 •

edited

Loading