-
Notifications
You must be signed in to change notification settings - Fork 358
Fix(tests): Address some test flakiness #5209
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
Changes from all commits
a9d375c
9876f20
cef45b9
44791c5
1905985
1eb2ac1
b049d5c
0145b40
f2e1702
e5f4287
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -212,6 +212,39 @@ def pytest_collection_modifyitems(items, *args, **kwargs): | |
| item.add_marker("fast") | ||
|
|
||
|
|
||
| @pytest.hookimpl(hookwrapper=True, tryfirst=True) | ||
| def pytest_runtest_makereport(item: pytest.Item, call: pytest.CallInfo): | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be the only hook that can catch the I tried both |
||
| # The tmp_path fixture frequently throws errors like: | ||
| # - KeyError: <_pytest.stash.StashKey object at 0x79ba385fe1a0> | ||
| # in its teardown. This causes pytest to mark the test as failed even though we have zero control over this behaviour. | ||
| # So we log/swallow that particular error here rather than raising it | ||
|
|
||
| # note: the hook always has to yield | ||
| outcome = yield | ||
|
|
||
| # we only care about tests that used the tmp_path fixture | ||
| if "tmp_path" not in getattr(item, "fixturenames", []): | ||
| return | ||
|
|
||
| result: pytest.TestReport = outcome.get_result() | ||
|
|
||
| if result.when != "teardown": | ||
| return | ||
|
|
||
| # If we specifically failed with a StashKey error in teardown, mark the test as passed | ||
| if result.failed: | ||
| exception = call.excinfo | ||
| if ( | ||
| exception | ||
| and isinstance(exception.value, KeyError) | ||
| and "_pytest.stash.StashKey" in repr(exception) | ||
| ): | ||
| result.outcome = "passed" | ||
| item.add_report_section( | ||
| "teardown", "stderr", f"Ignored tmp_path teardown error: {exception}" | ||
| ) | ||
|
|
||
|
|
||
| # Ignore all local config files | ||
| @pytest.fixture(scope="session", autouse=True) | ||
| def ignore_local_config_files(): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ gateways: | |
| type: duckdb | ||
| catalogs: | ||
| memory: ':memory:' | ||
| testing: 'testing.duckdb' | ||
| testing: "{{ var('tmp_path') }}/testing.duckdb" | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was causing flakiness in The This causes it to be re-used between tests and potentially accessed in parallel from multiple workers although there is an |
||
|
|
||
| # Databases with docker images available | ||
| inttest_trino_hive: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,15 +27,15 @@ | |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was session-scoped because its predecessor was session scoped and the session scope got retained through an earlier refactor. However, I was encountering some very hard-to-pin-down issues that I have seen a bunch of times regarding That looks like a concurrency issue and I had a theory it was related to session and function scoped fixtures being mixed. So i've made this function scoped because I couldnt see a strong reason for it to be session scoped |
||
| def config() -> Config: | ||
| @pytest.fixture | ||
| def config(tmp_path: pathlib.Path) -> Config: | ||
| return load_config_from_paths( | ||
| Config, | ||
| project_paths=[ | ||
| pathlib.Path("examples/wursthall/config.yaml"), | ||
| pathlib.Path(os.path.join(os.path.dirname(__file__), "config.yaml")), | ||
| ], | ||
| personal_paths=[pathlib.Path("~/.sqlmesh/config.yaml").expanduser()], | ||
| variables={"tmp_path": str(tmp_path)}, | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -89,7 +89,9 @@ def _create(engine_name: str, gateway: str) -> EngineAdapter: | |
|
|
||
| @pytest.fixture | ||
| def create_test_context( | ||
| request: FixtureRequest, create_engine_adapter: t.Callable[[str, str], EngineAdapter] | ||
| request: FixtureRequest, | ||
| create_engine_adapter: t.Callable[[str, str], EngineAdapter], | ||
| tmp_path: pathlib.Path, | ||
| ) -> t.Callable[[IntegrationTestEngine, str, str, str], t.Iterable[TestContext]]: | ||
| def _create( | ||
| engine: IntegrationTestEngine, gateway: str, test_type: str, table_format: str | ||
|
|
@@ -103,6 +105,7 @@ def _create( | |
| engine_adapter, | ||
| f"{engine.engine}_{table_format}", | ||
| gateway, | ||
| tmp_path=tmp_path, | ||
| is_remote=is_remote, | ||
| ) | ||
|
|
||
|
|
||
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 to make Databricks fail early with the cluster start error rather than time out for 40mins trying to run tests