-
Notifications
You must be signed in to change notification settings - Fork 85
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
Update Required e2e Tests #1296
Update Required e2e Tests #1296
Conversation
32e8bcb
to
955b37f
Compare
@@ -124,7 +124,7 @@ Feature: BigQueryExecute - Verify data transfer using BigQuery Execute plugin | |||
Then Verify the pipeline status is "Succeeded" | |||
Then Verify 1 records inserted in BigQuery table: "bqSourceTable" with query "bqExecuteCountDMLInsert" | |||
|
|||
@BQ_SOURCE_TEST @BQ_EXECUTE_UPSERT_SQL @BQExecute_Required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removing this test from required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific reason, Added this one Verify BQExecute plugin functionality for DDL query - Create table
to required list so to balance the build time removed this one. (This feature file already have 2 tests marked as required)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have the data how much time bq execute
required tests takes to execute in comparison to big ones like gcs
and bigquery
? If it is smaller then I think it should be fine to add it back because the overall execution time will remain the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran the test on local it takes less than 2 minutes to run, In that case I believe its okay to leave it so added it back (71afa4f)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash commits before merge.
71afa4f
to
259c0d8
Compare
Squashed the commits. |
259c0d8
to
52b227a
Compare
Updating the required e2e tests to use them for Sanity Testing on CDF.
Note - Increasing the ratio of Run Time Scenarios to cover more use cases, Product team should suggest if any other factor should be taken into account for Sanity Builds.