-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
MINOR: fix mssql integration test #17923
Conversation
…similar issue)
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Quality Gate passed for 'open-metadata-ingestion'Issues Measures |
@@ -22,24 +23,55 @@ | |||
from ..conftest import ingestion_config as base_ingestion_config | |||
|
|||
|
|||
@pytest.fixture(scope="module") |
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.
More a question on my end. Wouldn't scope=module
make it so the fixture would be destroyed at the end of the last test in sql_server/test_lineage.py
for example?
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.
Indeed. With containers a best practice is to have session (since its most expensive to setup) level fixtures for the containers and module level fixtures for data entities like databases or tables.
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.
But for this case the data stays static throughout the tests so its fine like this.
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
use sql server 2022 due to something like microsoft/mssql-docker#881 or microsoft/mssql-docker#441
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>