-
Notifications
You must be signed in to change notification settings - Fork 33
fix: run compliance tests with nox #26
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
| entry_points={ | ||
| "sqlalchemy.dialects": [ | ||
| "spanner = google.cloud.sqlalchemy_spanner:SpannerDialect" | ||
| "spanner.spanner = google.cloud.sqlalchemy_spanner:SpannerDialect" |
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'll break the package. This identifier is used to register dialect in Python modules system. With this change we'll have to connect like:
engine = create_engine(
"spanner.spanner:///projects/project-id/instances/instance-id/databases/database-id"
)And I don't think it'll work at all.
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.
I think it's working fine as you can see in SQLAlchemy spanner dialect test.
The url is spanner+spanner:///projects/appdev-soda-spanner-staging/instances/sqlalchemy-dialect-test/databases/compliance-test, which is perfectly working now.
spanner.spanner is used only to fetch the Dialect class. New version of sqlalchemy used by default spanner+spanner and replace + to . to fetch the entry-point or dialect class.
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.
The other way to overcome the failure is to override the method which added or forcefully converted spanner to spanner+spanner.
The method is generate_driver_url and path is sqlalchemy/testing/provision.py
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.
I've checked the tests locally, there is no failure at all. These changes are not needed.
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.
For now compliance tests are failing in you latest opened PR
https://github.com/cloudspannerecosystem/python-spanner-sqlalchemy/pull/27/checks?check_run_id=2119935389
Cloud you help me to solve that issue?
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.
Currently I am getting the same error as @HemangChothani which is also showing up on the Github Action. I'm not sure why it didn't show up before given that it does seem to be a issue with noxfile.py.
Pinning to an older version of sqlalchemy does not fix the issue on my end.
@IlyaFaer How are you running the compliance tests locally? Are you using the nox command? If not, please post the commands that you are running here so we can update the noxfile to match.
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.
@larkee In setup.py file I replace dependencies = ["sqlalchemy>=1.1.13"] to dependencies = ["sqlalchemy>=1.1.13, <=1.3.23"] and it works for me.
The reason of failing the command is they have implemented new method generate_driver_url() and path is sqlalchemy/testing/provision.py to generate the url for engine. In that method they replace spanner to spanner+spanner and going to find entry point of the library, in older version there was a spanner as a drive-name so they found perfectly, but in the new version they replace + with . and going to find entry point with spanner.spanner but we have spanner in setup file so it returns None that's why it's failing.
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.
@larkee, I'm executing tests locally just as usual, the only difference - I don't install packages on every nox session run, I'm using the local ones:
But if I'll uncomment these installing lines in nox session function, everything will still work.
As a simple select-script (executed without nox) is working fine with any version of SQLAlchemy, we can say registering the dialect should not to be changed (that would be strange, if SQLAlchemy team would change dialect registering mechanism in 1.4 version - it would break all the dialects). But nox session is failing, because registering mechanism is not able to find the Spanner dialect class. That makes me think the problem is in installing spanner-sqlalchemy package by nox.
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.
@IlyaFaer I have tried with manually installed spanner-sqlalchemy and SQLAlchemy and run pytest command manually (WIthout nox) and still sqlalchemy raising the same error. I think they have changed few things in testing procedure.
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.
I can confirm that updating setup.py with:
dependencies = ["sqlalchemy>=1.1.13, <=1.3.23", "google-cloud-spanner>=3.0.0"]
fixes the issue. The update to entry point is unneeded.
Two additional notes:
- The
compliance_testsession does not need to explicitly installsqlalchemyorgoogle-cloud-spannerbecause they are automatically install bysession.install("-e", ".")which installsspanner-sqlalchemyusingsetup.py create_test_databaseuses the project set by eitherGOOGLE_CLOUD_PROJECTorPROJECT_IDor defaults toemulator-test-projectbut the connection string defined insetup.cfgusing the prod project.
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.
The issues definitely seems to be sqlalchemy v1.4.0 since it was released yesterday and that's when this issue started appearing so I'm happy for the version to be pinned to sqlalchemy>=1.1.13, <=1.3.23 with no other changes needed 👍
|
Closing this PR in favor of #31 |

Fixes #25