-
Notifications
You must be signed in to change notification settings - Fork 4
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
Avoid running SQLAlchemy schema tests with Ingres dialect #50
Conversation
I am suspicious about the huge jump in skipped tests when setting
I think it will be important to run the suite with and without the setting and review the 521 tests that are affected to see they are indeed skipped due to the alternate schema issue and not some side effect. |
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.
Looks good.
Needs explanation text (that you wrote in the PR ) in the readme.
Minor tweak suggested for quoting.
README.testsuite.md
Outdated
|
||
## SQLAlchemy Dialect Compliance Suite | ||
### Overview and Setup | ||
SQLAlchemy includes a [dialect compliance suite](https://github.com/sqlalchemy/sqlalchemy/tree/main/lib/sqlalchemy/testing/suite) that is usable by third party libraries, in the source tree at lib/sqlalchemy/testing/suite. There's no need for a third party dialect to run through SQLAlchemy's full testing suite, as a large portion of these tests do not have dialect-sensitive functionality. The "dialect compliance suite" should be viewed as the primary target for new dialects. |
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.
quote this, i.e. prefix with >
README.testsuite.md
Outdated
test_schema | ||
test_schema_2 (only used on PostgreSQL and mssql) | ||
|
||
Please refer to your vendor documentation for the proper syntax to create these namespaces - the database user must have permission to create and drop tables within these schemas. Its perfectly fine to run the test suite without these namespaces present, it only means that a handful of tests which expect them to be present will fail. |
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.
quote these too, makes it more clear whats a quote (and where from) and our notes
README.testsuite.md
Outdated
[changelog_14.rst](https://github.com/sqlalchemy/sqlalchemy/blob/main/doc/build/changelog/changelog_14.rst) | ||
|
||
**Ingres Dialect Behavior** | ||
The Ingres dialect disables the ability to run tests that use alternate schemas with the setting `supports_schemas = False` in class `IngresDialect`. |
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 is a good statement, it needs an explanation adding (steal the text from your PR, that text is perfect).
lib/sqlalchemy_ingres/base.py
Outdated
@@ -387,6 +387,7 @@ class IngresDialect(default.DefaultDialect): | |||
supports_empty_insert = False | |||
supports_empty_insert = False | |||
supports_statement_cache = False # NOTE this is not actually picked up by SA warning code, _generate_cache_attrs() checks dict of subclass, not the entire class | |||
supports_schemas = False |
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.
Recommend having a comment here, could simple say "see readme.testsuite.md for details"
@clach04 Thanks for your review. I added text from the PR to the new README and cleaned it up a bit, including your suggestions for making the quoted text distinct that is taken from the SQLAlchemy repository pages. Please let me know of additional suggestions. Thanks. |
Discussed with @hab6 - he'd like to hold off merge due to >50% tests being skipped. Want to review |
Focusing solely on the tests in dialect compliance suite class ComponentReflectionTest: Without the flag Results of running ComponentReflectionTest class
|
When running the SQLAlchemy dialect compliance suite, quite a few tests would fail with some form of the following error:
The reason for the error is that the affected tests attempt to create and drop tables owned under schemas different than the current user.
Ingres does not support this kind of operation except in rare instances where the current user has security administrator and operator privilege or who is the DBA of the database. The user must then use the
SET SESSION AUTHORIZATION ...
command to set the effective user for the current session.Per discussion in 11366 and additional research in internal ticket II-14148, the setting
supports_schemas = False
will be added to the IngresDialect class to avoid execution of tests that use alternate schemas.The file README.testsuite.md is being added to provide information on how to run the dialect compliance suite and also provide information about the issue of running tests that contain alternate schemas.
Testing
supports_schemas = False