-
Notifications
You must be signed in to change notification settings - Fork 33
test: fix numeric compliance test #29
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
Conversation
larkee
left a comment
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 a lot of skipping relating to decimal which I'm concerned about. Would the changes included in #33 be relevant here?
|
All the tests passes once googleapis/python-spanner#290 PR merged. |
…n-spanner-sqlalchemy into numeric_compliance_test
…n-spanner-sqlalchemy into numeric_compliance_test
|
Tests look good to me. LGTM. |
vi3k6i5
left a comment
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.
Test changes look good to me. Please wait for Skylar's approval before merging.
larkee
left a comment
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.
LGTM other than the nit
|
Failed test will be resolved by PR #62 |
test/test_suite.py
Outdated
| of 38 and scale of 9. | ||
| """ | ||
| self._do_test( | ||
| Numeric(precision=18, scale=12), |
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.
We are overriding a number of tests because Spanner does not support scale>9. But these tests still uses Numeric with scale=12 or scale=14. I think it makes sense to change these Numerics to use scale=9. WDYT?
No description provided.