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.

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

Wrangler plugin e2e tests. #650

Conversation

priyabhatnagar25
Copy link
Contributor

Wrangler plugin e2e tests.

@Vipinofficial11 Vipinofficial11 added the build Triggers unit test build label Jul 25, 2023
@sau42shri sau42shri requested a review from vanathi-g July 26, 2023 06:14
@Vipinofficial11 Vipinofficial11 added build Triggers unit test build and removed build Triggers unit test build labels Jul 26, 2023
Copy link
Contributor

@vanathi-g vanathi-g left a comment

Choose a reason for hiding this comment

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

Seems like the build is failing because the workflow expects a plugin json file to be available but wrangler-core is not the module that generates jar + json files. wrangler-transform is the module with the actual wrangler plugin code.

@vanathi-g
Copy link
Contributor

Try removing wrangler-core from the workflow module config, wrangler-transform has a dependency on wrangler-core anyways

@BQ_SOURCE_TEST @BQ_SINK_TEST
Scenario: To verify User is able to run a pipeline using the fill null and send to error directives in the wrangler plugin
Given Open Datafusion Project to configure pipeline
Then Click on the Plus Green Button to import the pipelines
Copy link
Contributor

Choose a reason for hiding this comment

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

Unable to run the tests locally as it says this step (and a few others) are missing. Am I missing some config?

Choose a reason for hiding this comment

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

A PR has been raised for the steps in cdap-e2e-framework that are required for wrangler test scenarios. (cdapio/cdap-e2e-tests#193).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pom.xml Outdated
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.0.1-jre</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

edit the guava.version variable and use here

Choose a reason for hiding this comment

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

Using '${guava.version}'

Copy link
Contributor

Choose a reason for hiding this comment

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

update the value of guava.version to 31.0.1-jre

Copy link
Contributor

Choose a reason for hiding this comment

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

@vanathi-g should we update the global variable of guava version at L#103

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
<version>3.0.0-M5</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

This version is causing problems while trying to build locally. Verify once and refer to the config in database-plugins repo:
https://github.com/data-integrations/database-plugins/blob/develop/pom.xml#L634

<version>3.0.0</version>
<dependencies>
	<dependency>
		<groupId>org.apache.maven.surefire</groupId>
		<artifactId>surefire-junit47</artifactId>
		<version>3.0.0</version>
	</dependency>
</dependencies>

Choose a reason for hiding this comment

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

I upgraded the versions, but the build is still failing due to an error in generating the cucumber report file (None report file was added!). I'm looking into and debugging it.

pom.xml Show resolved Hide resolved
@poojantcs
Copy link

Github actions build is currently failing as these tests require new steps in the cdap-e2e-tests repository. Once these new steps are merged in the framework repository then the Wrangler tests would be executed again.

PR link for the cdap-e2e-tests: cdapio/cdap-e2e-tests#193

InterruptedException, URISyntaxException {
Map<String, JsonObject> bigQueryMap = new HashMap<>();
Map<String, JsonObject> fileMap = new HashMap<>();
Path importexpectedfile = Paths.get(ValidationHelper.class.getResource("/" + fileName).toURI());
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use camel case? importExpectedFile

Choose a reason for hiding this comment

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

Renamed it to --> 'importExpectedFile'

@AnkitCLI AnkitCLI force-pushed the Import_e2e_Wrangler_plugin branch 2 times, most recently from 32735a4 to d98da22 Compare August 21, 2023 09:13
getBigQueryTableData(table, bigQueryMap);
getFileData(importExpectedFile.toString(), fileMap);

boolean isMatched = matchJsonMaps(bigQueryMap, fileMap);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use bigqueryMap.equals(fileMap) and remove the matchJsonMaps function? The map equals method also functionally does the same thing (compare keys and corresponding values)

Copy link
Contributor

Choose a reason for hiding this comment

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

done removed that function

Copy link
Contributor

@vanathi-g vanathi-g left a comment

Choose a reason for hiding this comment

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

LGTM, please squash commits and merge.

The RunTime.feature class may have to be renamed in the next PR depending on what additional directive test cases are being added.

@@ -0,0 +1,9 @@
insert into `DATASET.TABLE_NAME` (ID, name, email, Customer_id, First_name, Last_name, Age, Address, Col1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the "body" column is used in the second test (parse csv, fill null, send to error) update the data to reflect an actual csv string.

Also, update the address column to have one null value to ensure the send-to-error if empty directive that is used is working correctly.

Choose a reason for hiding this comment

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

Done.

"properties": {
"field": "*",
"precondition": "false",
"directives": "parse-as-csv :body ',' true\ncopy :Customer_id :Customer_id_copy true\ndrop :Address\nset-column :name_count string:length(name)",
Copy link
Contributor

Choose a reason for hiding this comment

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

In the directive:
parse-as-csv :body ',' true

the 'true' argument is used to take the first value of the column as the header. There is actually no need to use this directive in this test. The current test has the copy directive's force argument set to true, so change the destination column to an existing column and validate the result accordingly.

Fix the usage in the other files as well to account for the first row being dropped in the results.

@vanathi-g
Copy link
Contributor

I would suggest adding just one .feature file per PR which tests atleast 7-8 directives in the same pipeline. For example, the directives can be grouped as such -

parser - 1 (ensure the actual data is in the format being parsed)
column operations - 4
row operations - 3

Can refer to this to get the groupings: https://cdap.atlassian.net/wiki/spaces/DOCS/pages/380600349/Directives

Change this PR to have just one scenario to begin with and ensure the data in BQ also reflects the behaviour we want to test, thanks!

@AnkitCLI
Copy link
Contributor

AnkitCLI commented Sep 6, 2023

closing pull request as new PR is raised for Wrangler directives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Triggers unit test build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants