apply suggestions from code review

Author: timrid

.vscode/settings.json

@@ -4,5 +4,7 @@
         "tests"
     ],
     "python.testing.unittestEnabled": false,
-    "python.testing.pytestEnabled": true
+    "python.testing.pytestEnabled": true,
+    "python-envs.defaultEnvManager": "ms-python.python:venv",
+    "python-envs.pythonProjects": []
 }

tests/conftest.py

@@ -1,9 +1,11 @@
 # Note that this conftest file exists at the package level as it is needed to configure
 # the JVM for docstrings at the package level.
-from collections.abc import Generator
-from pathlib import Path
 import logging
 import tempfile
+import textwrap
+from collections.abc import Generator
+from pathlib import Path
+
 import pytest
 
 import chaquopy_stubgen
@@ -22,10 +24,11 @@ def jvm():
     import jpype.imports  # type: ignore
 
     try:
-       yield jpype
+        yield jpype
     finally:
         jpype.shutdownJVM()
 
+
 @pytest.fixture(scope="session")
 def stub_dir(jvm) -> Generator[Path, None, None]:
     # logging.basicConfig(level="DEBUG")
@@ -47,7 +50,20 @@ def stub_dir(jvm) -> Generator[Path, None, None]:
 
 
 @pytest.fixture(scope="session")
-def mypy_project_dir() -> Generator[Path, None, None]:
+def mypy_project_dir(stub_dir: Path) -> Generator[Path, None, None]:
     with tempfile.TemporaryDirectory() as tmp:
-        yield Path(tmp)
-
+        project_dir = Path(tmp)
+        with project_dir.joinpath("pyproject.toml").open("w") as f:
+            f.write(
+                textwrap.dedent(
+                    f"""\
+                    [tool.mypy]
+                    mypy_path = "{stub_dir.absolute()}"
+
+                    [[tool.mypy.overrides]]
+                    module = "java.*"
+                    ignore_errors = true
+                    """
+                )
+            )
+        yield project_dir

tests/mypy_helper.py

@@ -2,7 +2,7 @@
 import json
 import logging
 import os
-import textwrap
+import re
 import tokenize
 from collections.abc import Generator
 from contextlib import contextmanager
@@ -46,24 +46,9 @@ def _parse_mypy_jsonl_output(mypy_stdout: str) -> list[MypyMessage]:
 
 def run_mypy(
     project_dir: Path,
-    extra_stub: Path,
     test_code: str,
 ) -> str:
     """Run mypy on the given test code and return the mypy stdout."""
-    with project_dir.joinpath("pyproject.toml").open("w") as f:
-        f.write(
-            textwrap.dedent(
-                f"""\
-                [tool.mypy]
-                mypy_path = "{extra_stub.absolute()}"
-
-                [[tool.mypy.overrides]]
-                module = "java.*"
-                ignore_errors = true
-                """
-            )
-        )
-
     testfile_name = "testfile.py"
     with project_dir.joinpath(testfile_name).open("w") as f:
         f.write(test_code)
@@ -81,7 +66,6 @@ def run_mypy(
 
 def run_and_assert_mypy(
     project_dir: Path,
-    extra_stub: Path,
     test_code: str,
     expected_output: dict[str, str] | str,
 ):
@@ -94,7 +78,7 @@ def run_and_assert_mypy(
     - A raw string containing the expected mypy output. In this case, the function will compare
       the raw mypy stdout with the expected string.
     """
-    mypy_stdout = run_mypy(project_dir, extra_stub, test_code)
+    mypy_stdout = run_mypy(project_dir, test_code)
     if isinstance(expected_output, dict):
         mypy_output = _parse_mypy_jsonl_output(mypy_stdout)
         assert_mypy_json_output(test_code, mypy_output, expected_output)
@@ -122,9 +106,7 @@ def _parse_marked_lines_with_tokenize(code: str) -> dict[str, int]:
 
     for token in tokens:
         if token.type == tokenize.COMMENT:
-            # Token.string contains the comment including the #
-            import re
-
+            # token.string contains the comment including the #
             match = re.search(r"#\s*\*(\d+)", token.string)
             if match:
                 marker = f"*{match.group(1)}"
@@ -143,6 +125,11 @@ def assert_mypy_json_output(
     # Parse the code to find the marked lines
     marked_lines: dict[str, int] = _parse_marked_lines_with_tokenize(code)
 
+    # Validate that all expected markers are present in the code
+    missing_markers = set(expected_output.keys()) - set(marked_lines.keys())
+    if missing_markers:
+        assert False, f"Expected markers not found in code: {', '.join(sorted(missing_markers))}"
+
     # Group messages by line number
     result_by_line: dict[int, list[MypyMessage]] = {}
     for msg in mypy_output:
@@ -168,5 +155,5 @@ def assert_mypy_json_output(
         unexpected_msgs = []
         for line, msgs in result_by_line.items():
             for msg in msgs:
-                unexpected_msgs.append(_format_mypy_msg(msg))
-            assert False, f"Unexpected mypy message in line {line}:\n{unexpected_msgs}"
+                unexpected_msgs.append(f"line {line}: {_format_mypy_msg(msg)}")
+        assert False, "Unexpected mypy messages:\n" + "\n".join(unexpected_msgs)

tests/test_array_list.py

@@ -3,7 +3,7 @@
 from .mypy_helper import run_and_assert_mypy
 
 
-def test_array_list_valid(stub_dir: Path, mypy_project_dir: Path):
+def test_array_list_valid(mypy_project_dir: Path):
     code = """\
 from java.util import ArrayList
 
@@ -21,10 +21,10 @@ def test_array_list_valid(stub_dir: Path, mypy_project_dir: Path):
 note: Revealed type is "builtins.str"''',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_array_list_invalid(stub_dir: Path, mypy_project_dir: Path):
+def test_array_list_invalid(mypy_project_dir: Path):
     code = """\
 from java.util import ArrayList
 
@@ -40,10 +40,10 @@ def test_array_list_invalid(stub_dir: Path, mypy_project_dir: Path):
 hint:     def add(self, int: int | jint | Integer, e: str) -> None""",
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_array_list_no_implicit_conversion(stub_dir: Path, mypy_project_dir: Path):
+def test_array_list_no_implicit_conversion(mypy_project_dir: Path):
     code = """\
 from java.util import ArrayList
 
@@ -60,10 +60,10 @@ def test_array_list_no_implicit_conversion(stub_dir: Path, mypy_project_dir: Pat
 hint:     def [_ArrayList__E] ArrayList(self, collection: Collection[_ArrayList__E]) -> ArrayList[_ArrayList__E]""",
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_array_list_missing_type_annotation(stub_dir: Path, mypy_project_dir: Path):
+def test_array_list_missing_type_annotation(mypy_project_dir: Path):
     code = """\
 from java.util import ArrayList
 
@@ -74,10 +74,10 @@ def test_array_list_missing_type_annotation(stub_dir: Path, mypy_project_dir: Pa
         "*1": 'error: Need type annotation for "java_array_list"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_array_list_no_getitem(stub_dir: Path, mypy_project_dir: Path):
+def test_array_list_no_getitem(mypy_project_dir: Path):
     code = """\
 from java.util import ArrayList
 
@@ -89,4 +89,4 @@ def test_array_list_no_getitem(stub_dir: Path, mypy_project_dir: Path):
         "*1": 'error: Value of type "ArrayList[str]" is not indexable',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)

tests/test_callbacks.py

@@ -3,7 +3,7 @@
 from .mypy_helper import run_and_assert_mypy
 
 
-def test_bifunction_callback(stub_dir: Path, mypy_project_dir: Path):
+def test_bifunction_callback(mypy_project_dir: Path):
     code = """\
 import typing
 
@@ -63,10 +63,10 @@ def invalid_for_each_cb_2(k: str) -> None:
         "*2": 'error: Argument 1 to "PyBiConsumer" has incompatible type "Callable[[str], None]"; expected "Callable[[str, float], None]"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_predicate_callback(stub_dir: Path, mypy_project_dir: Path):
+def test_predicate_callback(mypy_project_dir: Path):
     code = """\
 import typing
 
@@ -135,10 +135,10 @@ def invalid_predicate_3(k: str) -> int:
         "*3": 'error: Argument 1 to "PyPredicate" has incompatible type "Callable[[str], int]"; expected "Callable[[str], bool]"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_stream_chaining_and_type_inference(stub_dir: Path, mypy_project_dir: Path):
+def test_stream_chaining_and_type_inference(mypy_project_dir: Path):
     code = """\
 import typing
 
@@ -238,4 +238,4 @@ def map_atomicint_to_bool(v: AtomicInteger) -> bool:
         "*6": 'note: Revealed type is "java.util.Set[java.util.concurrent.atomic.AtomicInteger]"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)

tests/test_enummap.py

@@ -3,7 +3,7 @@
 from .mypy_helper import run_and_assert_mypy
 
 
-def test_enum_map_valid(stub_dir: Path, mypy_project_dir: Path):
+def test_enum_map_valid(mypy_project_dir: Path):
     code = """\
 import typing
 from java.util import EnumMap
@@ -19,10 +19,10 @@ def test_enum_map_valid(stub_dir: Path, mypy_project_dir: Path):
         "*1": 'note: Revealed type is "java.util.EnumMap[java.util.concurrent.TimeUnit, Any]"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_enum_map_invalid(stub_dir: Path, mypy_project_dir: Path):
+def test_enum_map_invalid(mypy_project_dir: Path):
     code = """\
 import typing
 from java.util import EnumMap
@@ -40,4 +40,4 @@ def test_enum_map_invalid(stub_dir: Path, mypy_project_dir: Path):
         "*2": 'error: Argument 1 to "put" of "EnumMap" has incompatible type "str"; expected "TimeUnit"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)

tests/test_exception.py

@@ -3,7 +3,7 @@
 from .mypy_helper import run_and_assert_mypy
 
 
-def test_java_exception(stub_dir: Path, mypy_project_dir: Path):
+def test_java_exception(mypy_project_dir: Path):
     code = """\
 from java.lang import Exception
 java_exception = Exception("Testing")
@@ -17,4 +17,4 @@ def test_java_exception(stub_dir: Path, mypy_project_dir: Path):
         "*1": 'note: Revealed type is "java.lang.Exception"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)

tests/test_forward_declaration.py

@@ -3,7 +3,7 @@
 from .mypy_helper import run_and_assert_mypy
 
 
-def test_argument_type_declaration(stub_dir: Path, mypy_project_dir: Path):
+def test_argument_type_declaration(mypy_project_dir: Path):
     code = """\
 import typing
 
@@ -18,4 +18,4 @@ def foo(arg: "java.util.Formatter"):
         "*1": 'note: Revealed type is "java.util.Formatter"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)

tests/test_hashmap.py

@@ -3,7 +3,7 @@
 from .mypy_helper import run_and_assert_mypy
 
 
-def test_hash_map_valid(stub_dir: Path, mypy_project_dir: Path):
+def test_hash_map_valid(mypy_project_dir: Path):
     code = """\
 from java.util import HashMap
 
@@ -27,10 +27,10 @@ def test_hash_map_valid(stub_dir: Path, mypy_project_dir: Path):
         "*4": 'note: Revealed type is "builtins.float"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_hash_map_invalid(stub_dir: Path, mypy_project_dir: Path):
+def test_hash_map_invalid(mypy_project_dir: Path):
     code = """\
 from java.util import HashMap
 
@@ -45,4 +45,4 @@ def test_hash_map_invalid(stub_dir: Path, mypy_project_dir: Path):
         "*1": 'error: Argument 2 to "put" of "HashMap" has incompatible type "str"; expected "float"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)

tests/test_jarray.py

@@ -3,7 +3,7 @@
 from .mypy_helper import run_and_assert_mypy
 
 
-def test_java_array_as_return_type(stub_dir: Path, mypy_project_dir: Path):
+def test_java_array_as_return_type(mypy_project_dir: Path):
     code = """\
 from java.lang import String
 
@@ -19,10 +19,10 @@ def test_java_array_as_return_type(stub_dir: Path, mypy_project_dir: Path):
         "*3": 'note: Revealed type is "java.chaquopy.JavaArrayJChar"',
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)
 
 
-def test_java_array_as_parameter(stub_dir: Path, mypy_project_dir: Path):
+def test_java_array_as_parameter(mypy_project_dir: Path):
     code = """\
 from java.lang import String
 from java import jarray, jchar, jint, jbyte
@@ -62,4 +62,4 @@ def test_java_array_as_parameter(stub_dir: Path, mypy_project_dir: Path):
 hint:     def String(self, stringBuilder: StringBuilder) -> String""",
     }
 
-    run_and_assert_mypy(mypy_project_dir, stub_dir, code, expected_mypy_output)
+    run_and_assert_mypy(mypy_project_dir, code, expected_mypy_output)