Skip to content

Commit

Permalink
test(marks): add broken mark, remove unneeded markers
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
gforsyth authored and cpcloud committed Mar 10, 2022
1 parent 0bb3de1 commit f7915d0
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 46 deletions.
42 changes: 15 additions & 27 deletions ibis/backends/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand Down Expand Up @@ -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):
Expand Down
11 changes: 5 additions & 6 deletions ibis/backends/tests/test_aggregation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
6 changes: 4 additions & 2 deletions ibis/backends/tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand Down
12 changes: 7 additions & 5 deletions ibis/backends/tests/test_numeric.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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
):
Expand Down
2 changes: 1 addition & 1 deletion ibis/backends/tests/test_temporal.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion ibis/backends/tests/test_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
),
Expand Down
5 changes: 1 addition & 4 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit f7915d0

Please sign in to comment.