[GitHub] [iceberg] Fokko commented on a diff in pull request #7136: Python: Make Python CI collect most of the unit tests and Fix CI name conflict

["Fokko (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org