Skip to content

Conversation

@kaxil
Copy link
Member

@kaxil kaxil commented Jan 7, 2022

This PR improves test_run_storing_query_ids_extra test for Snowflake Hook.

  • Redundant testing of Snowflake.get_conn (This is already tested in test_get_conn_should_call_connect)
  • The expected values were derived from input, so you can't easily fail the test ! I have passed expected results and input separately
  • We were passing duplicate values in query_ids. I have fixed that by changing the code to only access cur.sfqid once in a variable and reuse that variable for logging and returning.

Co-authored-by: bharanidharan14 94612827+bharanidharan14@users.noreply.github.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added area:providers provider:snowflake Issues related to Snowflake provider labels Jan 7, 2022
@kaxil
Copy link
Member Author

kaxil commented Jan 7, 2022

cc @bharanidharan14

@kaxil kaxil requested a review from ephraimbuddy January 7, 2022 12:27
This PR improves `test_run_storing_query_ids_extra` test for Snowflake Hook.

- Redundant testing of `Snowflake.get_conn` (This is already tested in `test_get_conn_should_call_connect`)
- The expected values were derived from input, so you can't easily fail the test ! I have passed expected results and input separately
- We were passing duplicate values in `query_ids`. I have fixed that by changing the code to only access `cur.sfqid` once in a variable and reuse that variable for logging and returning.

Co-Authored-By: bharanidharan14 <94612827+bharanidharan14@users.noreply.github.com>
@kaxil kaxil force-pushed the improve-snowflake-tests branch from d39280e to 9eb461e Compare January 7, 2022 15:14
@kaxil kaxil requested a review from eladkal January 7, 2022 15:25
@github-actions
Copy link

github-actions bot commented Jan 7, 2022

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jan 7, 2022
@kaxil kaxil merged commit 9ea459a into apache:main Jan 7, 2022
@kaxil kaxil deleted the improve-snowflake-tests branch January 7, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:snowflake Issues related to Snowflake provider

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants