Fokko commented on code in PR #7136:
URL: https://github.com/apache/iceberg/pull/7136#discussion_r1143016966


##########
python/tests/conftest.py:
##########
@@ -80,6 +80,12 @@
 )
 
 
+def pytest_collection_modifyitems(items: List[pytest.Item]) -> None:
+    for item in items:
+        if not any(item.iter_markers()):
+            item.add_marker("unmarked")

Review Comment:
   Thanks, I don't know why this got lost.



##########
python/Makefile:
##########
@@ -27,7 +27,7 @@ lint:
 
 test:
        poetry run coverage run --source=pyiceberg/ -m pytest tests/ -m 
unmarked ${PYTEST_ARGS}
-       poetry run coverage report -m --fail-under=90
+       poetry run coverage report -m --fail-under=88

Review Comment:
   @JonasJ-ap I think this only runs the tests that are marked as `unmarked` 
and checks the code coverage. How about adding another target in the Makefile 
that will run all the test of all the markers (`unmarked`, `s3`, `adlfs`, and 
`gcs` soon), and only add the code coverage check there. Right now we expect 
all the test suites to cover 90%+, which is not really needed.



##########
python/tests/io/test_pyarrow.py:
##########
@@ -450,17 +450,11 @@ def test_expr_not_null_to_pyarrow(bound_reference: 
BoundReference[str]) -> None:
 
 
 def test_expr_is_nan_to_pyarrow(bound_double_reference: BoundReference[str]) 
-> None:
-    assert (
-        repr(expression_to_pyarrow(BoundIsNaN(bound_double_reference)))
-        == "<pyarrow.compute.Expression (is_null(foo, {nan_is_null=true}) and 
is_valid(foo))>"
-    )
+    assert repr(expression_to_pyarrow(BoundIsNaN(bound_double_reference))) == 
"<pyarrow.compute.Expression is_nan(foo)>"
 
 
 def test_expr_not_nan_to_pyarrow(bound_double_reference: BoundReference[str]) 
-> None:
-    assert (
-        repr(expression_to_pyarrow(BoundNotNaN(bound_double_reference)))
-        == "<pyarrow.compute.Expression invert((is_null(foo, 
{nan_is_null=true}) and is_valid(foo)))>"
-    )
+    assert repr(expression_to_pyarrow(BoundNotNaN(bound_double_reference))) == 
"<pyarrow.compute.Expression invert(is_nan(foo))>"

Review Comment:
   Looks good, thanks!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to