-
Notifications
You must be signed in to change notification settings - Fork 122
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
test: improve to_gbq
logic unit test coverage
#449
Conversation
…ssue392-unit-test-100
…-bigquery-pandas into issue392-unit-test-100
…-bigquery-pandas into issue392-unit-test-100
to_gbq
logic unit test coverage
@@ -1057,7 +1063,7 @@ def to_gbq( | |||
DeprecationWarning, | |||
stacklevel=2, | |||
) | |||
elif api_method == "load_csv": | |||
else: |
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.
There's already a check above for known api_methods
, so this was an impossible branch.
@@ -1122,12 +1128,14 @@ def to_gbq( | |||
) | |||
elif if_exists == "replace": | |||
connector.delete_and_recreate_table(dataset_id, table_id, table_schema) | |||
elif if_exists == "append": | |||
else: |
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.
There's already a check above for known if_exists
, so this was an impossible branch.
if not pandas_gbq.schema.schema_is_subset(original_schema, table_schema): | ||
raise InvalidSchema( | ||
"Please verify that the structure and " | ||
"data types in the DataFrame match the " | ||
"schema of the destination table." | ||
"schema of the destination table.", | ||
table_schema, |
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.
Towards #349
.coveragerc
Outdated
@@ -22,7 +22,7 @@ omit = | |||
google/cloud/__init__.py | |||
|
|||
[report] | |||
fail_under = 89 | |||
fail_under = 95 |
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.
Still not 100, but it's solid progress. I don't want this PR to get too long. Mostly I started working on unit tests because they're just mindless enough that I can handle it while I'm sick this week.
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.
It's fine to merge a meaningful piece of work into main
, it doesn't have to be all-or-nothing. This PR by itself is already a significant improvement.
(I will review this tomorrow, did not manage to go through everything today)
def get_table(table_ref_or_id, **kwargs): | ||
return google.cloud.bigquery.Table(table_ref_or_id) | ||
|
||
mock_client.get_table.side_effect = get_table |
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.
This was messing with the to_gbq
tests, so moved to test_gbq.py
(and potentially to a test_read_gbq.py
in future.
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.
Looking good, not much to add really, one or two remarks aside.
One of the system tests might need an adjustment, though, it does not look like flakiness.
I've removed the failing system test. The behavior of |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Towards #392 🦕
Based on #443