From f7915d0449c0a5aefe19859e063b0051a669f2e2 Mon Sep 17 00:00:00 2001 From: Gil Forsyth Date: Thu, 10 Mar 2022 12:00:43 -0500 Subject: [PATCH] test(marks): add broken mark, remove unneeded markers This adds a new marker named `broken` for marking broken tests (:wink:). This should be used sparingly and is designed for scenarios where a user has added a new test and has exposed previously broken functionality that is unrelated to their feature -- in that case, we don't want to impose the necessity of fixing it on them. I've also cleaned up a few of the older markers that were used in only a few spots and replaced them with `notyet`, `notimpl`, and `never`. The markers removed are: `ddl_only` - unused `skip_missing_feature` - unused `xfail_backends` - few replacements made `only_on_backends` - few replacements made I've left in `skip_backends` because there are a few tests (which I would like to fix) that crash the test suite if they aren't skipped on particular backends (mostly memory explosions with pandas and dask). --- ibis/backends/conftest.py | 42 +++++++++---------------- ibis/backends/tests/test_aggregation.py | 11 +++---- ibis/backends/tests/test_client.py | 6 ++-- ibis/backends/tests/test_numeric.py | 12 ++++--- ibis/backends/tests/test_temporal.py | 2 +- ibis/backends/tests/test_window.py | 2 +- pyproject.toml | 5 +-- 7 files changed, 34 insertions(+), 46 deletions(-) diff --git a/ibis/backends/conftest.py b/ibis/backends/conftest.py index 181b3b5de435..0ad3d24f1977 100644 --- a/ibis/backends/conftest.py +++ b/ibis/backends/conftest.py @@ -183,37 +183,10 @@ def pytest_runtest_call(item): backend = next(iter(backend)) - for marker in item.iter_markers(name="only_on_backends"): - if backend not in marker.args[0]: - pytest.skip( - f"only_on_backends: {backend} is not in {marker.args[0]} " - f"{nodeid}" - ) - for marker in item.iter_markers(name="skip_backends"): if backend in marker.args[0]: pytest.skip(f"skip_backends: {backend} {nodeid}") - for marker in item.iter_markers(name="xfail_backends"): - for entry in marker.args[0]: - if isinstance(entry, tuple): - name, reason = entry - else: - name = entry - reason = marker.kwargs.get("reason") - - if backend == name: - item.add_marker( - pytest.mark.xfail( - reason=reason or f'{backend} in xfail list: {name}', - **{ - k: v - for k, v in marker.kwargs.items() - if k != "reason" - }, - ) - ) - for marker in item.iter_markers(name='min_spark_version'): min_version = marker.args[0] if backend == 'pyspark': @@ -269,6 +242,21 @@ def pytest_runtest_call(item): ) ) + # Something has been exposed as broken by a new test and it shouldn't be + # imperative for a contributor to fix it just because they happened to + # bring it to attention -- USE SPARINGLY + for marker in item.iter_markers(name="broken"): + if backend in marker.args[0]: + reason = marker.kwargs.get("reason") + item.add_marker( + pytest.mark.xfail( + reason=reason or f"Feature is failing on {backend}", + **{ + k: v for k, v in marker.kwargs.items() if k != "reason" + }, + ) + ) + @pytest.fixture(params=_get_backends_to_test(), scope='session') def backend(request, data_directory): diff --git a/ibis/backends/tests/test_aggregation.py b/ibis/backends/tests/test_aggregation.py index 2320f1e2e7e3..a285dafc3303 100644 --- a/ibis/backends/tests/test_aggregation.py +++ b/ibis/backends/tests/test_aggregation.py @@ -238,7 +238,7 @@ def test_aggregate_grouped( lambda t, where: t.string_col.approx_nunique(), lambda t, where: t.string_col.nunique(), id='approx_nunique', - marks=pytest.mark.xfail_backends(['mysql', 'sqlite', 'pyspark']), + marks=pytest.mark.notyet(['mysql', 'sqlite', 'pyspark']), ), param( lambda t, where: t.double_col.arbitrary(how='first'), @@ -344,7 +344,7 @@ def test_group_concat(backend, alltypes, df, result_fn, expected_fn): ], ) @mark.notimpl(["pandas", "dask", "csv", "hdf5", "parquet"]) -@pytest.mark.xfail_backends([('pyspark', '#2130'), 'datafusion']) +@pytest.mark.notyet(["pyspark", "datafusion"]) def test_topk_op(backend, alltypes, df, result_fn, expected_fn): # TopK expression will order rows by "count" but each backend # can have different result for that. @@ -371,10 +371,9 @@ def test_topk_op(backend, alltypes, df, result_fn, expected_fn): ) ], ) -@pytest.mark.xfail_backends(['sqlite'], reason='Issue #2128') -@pytest.mark.xfail_backends( - [('mysql', 'Issue #2131'), ('postgres', 'Issue #2132')] -) +@pytest.mark.notyet(['sqlite'], reason='Issue #2128') +@pytest.mark.notyet(["mysql"], reason="Issue #2131") +@pytest.mark.notyet(["postgres"], reason="Issue #2132") @mark.notimpl(["datafusion", "duckdb", "pandas", "dask", "pyspark"]) def test_topk_filter_op(backend, alltypes, df, result_fn, expected_fn): # TopK expression will order rows by "count" but each backend diff --git a/ibis/backends/tests/test_client.py b/ibis/backends/tests/test_client.py index 9978d898d8ca..2e47cc31f340 100644 --- a/ibis/backends/tests/test_client.py +++ b/ibis/backends/tests/test_client.py @@ -150,7 +150,10 @@ def test_nullable_input_output(con, temp_table): assert t.schema().types[2].nullable -@mark.only_on_backends(["impala"]) +@mark.notimpl( + ["clickhouse", "datafusion", "duckdb", "mysql", "postgres", "sqlite"] +) +@mark.notyet(["pyspark"]) def test_create_drop_view(ddl_con, ddl_backend, temp_view): # setup table_name = 'functional_alltypes' @@ -293,7 +296,6 @@ def test_verify(ddl_backend, ddl_con): assert ddl_backend.api.verify(expr) -@mark.only_on_backends(["postgres"]) def test_not_verify(alchemy_con, alchemy_backend): # There is no expression that can't be compiled to any backend # Testing `not verify()` only for an expression not supported in postgres diff --git a/ibis/backends/tests/test_numeric.py b/ibis/backends/tests/test_numeric.py index 30e3c359de96..54a4cb0ca7f5 100644 --- a/ibis/backends/tests/test_numeric.py +++ b/ibis/backends/tests/test_numeric.py @@ -384,8 +384,6 @@ def test_mod(backend, alltypes, df): def test_floating_mod(backend, alltypes, df): - if not backend.supports_floating_modulus: - pytest.skip(f'{backend} does not support floating modulus operation') expr = operator.mod(alltypes.double_col, alltypes.smallint_col + 1) result = expr.execute() @@ -406,10 +404,11 @@ def test_floating_mod(backend, alltypes, df): 'double_col', ], ) +@pytest.mark.notyet( + ["datafusion", "duckdb", "mysql", "postgres", "pyspark", "sqlite"] +) @pytest.mark.parametrize('denominator', [0, 0.0]) def test_divide_by_zero(backend, alltypes, df, column, denominator): - if not backend.supports_divide_by_zero: - pytest.skip(f'{backend} does not support safe division by zero') expr = alltypes[column] / denominator expected = backend.default_series_rename(df[column].div(denominator)) result = expr.execute() @@ -427,7 +426,10 @@ def test_divide_by_zero(backend, alltypes, df, column, denominator): ], ) @pytest.mark.notimpl(["sqlite", "duckdb"]) -@pytest.mark.only_on_backends(["sqlite", "duckdb", "postgres", "mysql"]) +@pytest.mark.never( + ["clickhouse", "dask", "datafusion", "impala", "pandas", "pyspark"], + reason="Not SQLAlchemy backends", +) def test_sa_default_numeric_precision_and_scale( con, backend, dialects, default_precisions, default_scales ): diff --git a/ibis/backends/tests/test_temporal.py b/ibis/backends/tests/test_temporal.py index d1c8a92e575b..0f954625b946 100644 --- a/ibis/backends/tests/test_temporal.py +++ b/ibis/backends/tests/test_temporal.py @@ -60,7 +60,7 @@ def test_timestamp_extract(backend, alltypes, df, attr): @pytest.mark.notimpl(["datafusion", "clickhouse"]) -@pytest.mark.xfail_backends([('sqlite', "#2156"), ('pyspark', '#2159')]) +@pytest.mark.notyet(["sqlite", "pyspark"]) def test_timestamp_extract_milliseconds(backend, alltypes, df): expr = alltypes.timestamp_col.millisecond() result = expr.execute() diff --git a/ibis/backends/tests/test_window.py b/ibis/backends/tests/test_window.py index aa39bd715610..6658d010967b 100644 --- a/ibis/backends/tests/test_window.py +++ b/ibis/backends/tests/test_window.py @@ -140,7 +140,7 @@ def calc_zscore(s): .astype(bool) ), id='cumnotall', - marks=pytest.mark.xfail_backends( + marks=pytest.mark.notyet( ("duckdb", 'impala', 'postgres', 'mysql', 'sqlite'), reason="notall() over window not supported", ), diff --git a/pyproject.toml b/pyproject.toml index a1ce51d8e764..28c9663accb1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -155,14 +155,11 @@ markers = [ "geospatial: tests for geospatial functionality", "hdfs: Hadoop file system tests", "min_spark_version: backends tests that require a specific version of pyspark to pass", - "only_on_backends: run tests only on the provided backends", "skip_backends: skip tests on the provided backends", - "skip_missing_feature: skip tests with a known missing feature", - "xfail_backends: xfail tests on the provided backends", "notimpl: functionality that isn't implemented in ibis", "notyet: for functionality that isn't implemented in a backend", "never: tests for functionality that a backend is likely to never implement", - "ddl_only: tests for ddl-implementing backends", + "broken: test has exposed existing broken functionality", "sqlalchemy_only: tests for SQLAlchemy based backends", "clickhouse: ClickHouse tests", "dask: Dask tests",