Merge PR #703: Fix mypy errors in pytest_ofrak and enforce it in CI

Author: Aleksey Nogin

build_image.py

@@ -294,19 +294,26 @@ def create_dockerfile_finish(config: OfrakImageConfig) -> str:
     )
     dockerfile_finish_parts.append(f'RUN printf "{develop_makefile}" >> Makefile\n')
     dockerfile_finish_parts.append("RUN make $INSTALL_TARGET\n\n")
-    test_names = " ".join([f"test_{package_name}" for package_name in package_names])
-    finish_makefile = "\\n\\\n".join(
-        [
-            ".PHONY: test " + test_names,
-            "test: " + test_names,
-        ]
-        + [
-            f"test_{package_name}:\\n\\\n\t\\$(MAKE) -C {package_name} test"
-            for package_name in package_names
+    if config.install_target is InstallTarget.DEVELOP:
+        test_names = ["pytest_ofrak_inspect"] + [
+            f"test_{package_name}" for package_name in package_names
         ]
-        + ["\\n"]
-    )
-    dockerfile_finish_parts.append(f'RUN printf "{finish_makefile}" >> Makefile\n')
+        test_names_str = " ".join(test_names)
+        finish_makefile = "\\n\\\n".join(
+            [
+                ".PHONY: test " + test_names_str,
+                "test: " + test_names_str,
+                "pytest_ofrak_inspect:",
+                "\t\\$(MAKE) -C pytest_ofrak inspect",
+            ]
+            + [
+                line
+                for package_name in package_names
+                for line in (f"test_{package_name}:", f"\t\\$(MAKE) -C {package_name} test")
+            ]
+            + ["\\n"]
+        )
+        dockerfile_finish_parts.append(f'RUN printf "{finish_makefile}" >> Makefile\n')
     if config.entrypoint is not None:
         dockerfile_finish_parts.append('SHELL ["/bin/bash", "-c"]\n')
         dockerfile_finish_parts.append(f"ENTRYPOINT {config.entrypoint}")

pytest_ofrak/CHANGELOG.md

@@ -0,0 +1,10 @@
+# Changelog
+All notable changes to `pytest-ofrak` will be documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
+
+## [0.1.0](https://github.com/redballoonsecurity/ofrak/compare/pytest-ofrak-v0.0.0...pytest-ofrak-v0.1.0)
+
+### Changed
+
+- Fix mypy type errors across the package ([#703](https://github.com/redballoonsecurity/ofrak/pull/703))

pytest_ofrak/Makefile

@@ -8,3 +8,7 @@ install:
 .PHONY: develop
 develop:
 	$(PYTHON) -m pip install -e . --config-settings editable_mode=compat
+
+.PHONY: inspect
+inspect:
+	$(PYTHON) -m mypy

pytest_ofrak/mypy.ini

@@ -0,0 +1,8 @@
+[mypy]
+files = src
+
+[mypy-synthol.*]
+ignore_missing_imports = True
+
+[mypy-xattr.*]
+ignore_missing_imports = True

pytest_ofrak/setup.py

@@ -21,7 +21,7 @@ def run(self):
 
 setuptools.setup(
     name="pytest-ofrak",
-    version="0.1.0rc0",
+    version="0.1.0rc1",
     description="Pytest fixtures and tests for OFRAK",
     packages=setuptools.find_packages("src"),
     package_dir={"": "src"},

pytest_ofrak/src/pytest_ofrak/mark.py

@@ -1,5 +1,5 @@
 import asyncio
-from typing import Mapping, Sequence, Type, Tuple
+from typing import Mapping, Sequence, Type
 import sys
 import pytest
 
@@ -16,7 +16,7 @@ async def _check_deps_installed(
 def _handle_skipif_missing_deps(
     deps_installed_data: Mapping[ComponentExternalTool, bool],
     components: Sequence[Type[AbstractComponent]],
-) -> Tuple[bool, str]:
+) -> "pytest.MarkDecorator":
     """
     :param deps_installed_data: a precomputed mapping of all tools to their installed status
     :params components: the components a test case uses

pytest_ofrak/src/pytest_ofrak/mock_component_types.py

@@ -1,20 +1,22 @@
 from abc import ABC
-from typing import Tuple
+from typing import Any, Optional, Tuple, Type
 
 from ofrak import Analyzer, Unpacker, Resource
+from ofrak.model.component_model import ComponentConfig
+from ofrak.resource_view import ResourceView
 
 
 class MockAnalyzer(Analyzer[None, Tuple], ABC):
     def __init__(self):
         super().__init__(None, None, None)
 
-    async def analyze(self, resource: Resource, config=None) -> Tuple:
+    async def analyze(self, resource: Resource, config: Optional[ComponentConfig] = None) -> Any:
         return ()
 
 
 class MockUnpacker(Unpacker[None]):
-    targets = ()
-    children = ()
+    targets: Tuple[Type[ResourceView], ...] = ()
+    children: Tuple[Type[ResourceView], ...] = ()
 
     def __init__(self):
         super().__init__(None, None, None, None)
@@ -24,8 +26,8 @@ async def unpack(self, resource, config=None):
 
 
 class MockRunnableUnpacker(Unpacker[None]):
-    targets = ()
-    children = ()
+    targets: Tuple[Type[ResourceView], ...] = ()
+    children: Tuple[Type[ResourceView], ...] = ()
 
     async def unpack(self, resource, config=None):
         pass

pytest_ofrak/src/pytest_ofrak/mock_library.py

@@ -98,7 +98,7 @@ class ITargetsCommonOutputsA(Analyzer[None, Tuple[AbstractionAttributesA]], ABC)
     id = b"TargetsCommonOutputsA"
 
     @abstractmethod
-    def analyze(
+    async def analyze(
         self, resource: Resource, config: Optional[ComponentConfig]
     ) -> Tuple[AbstractionAttributesA]:
         raise NotImplementedError()
@@ -113,11 +113,14 @@ class ITargetsQOutputsABC(
     ABC,
 ):
     targets = (AbstractionQ,)
-    outputs = (AbstractionAttributesA, AbstractionAttributesB, AbstractionAttributesC)
+    # Intentionally tests multi-element outputs tuple to exercise iteration logic
+    # in Analyzer.get_outputs_as_attribute_types(), even though the base class
+    # type annotation only specifies single-element tuples.
+    outputs = (AbstractionAttributesA, AbstractionAttributesB, AbstractionAttributesC)  # type: ignore[assignment]
     id = b"TargetsQOutputsABC"
 
     @abstractmethod
-    def analyze(
+    async def analyze(
         self, resource: Resource, config: Optional[ComponentConfig]
     ) -> Tuple[AbstractionAttributesA, AbstractionAttributesB, AbstractionAttributesC]:
         raise NotImplementedError()
@@ -133,19 +136,19 @@ class IWithoutImplementation(Analyzer[None, Tuple[AbstractionAttributesA]], ABC)
     id = b"WithoutImplementation"
 
     @abstractmethod
-    def analyze(
+    async def analyze(
         self, resource: Resource, config: Optional[ComponentConfig]
     ) -> Tuple[AbstractionAttributesA]:
         raise NotImplementedError()
 
 
 class AbstractionPUnpacker(MockUnpacker):
-    targets = [AbstractionP]
+    targets = (AbstractionP,)
 
 
 class AbstractionRUnpacker(MockUnpacker):
-    targets = [AbstractionR]
+    targets = (AbstractionR,)
 
 
 class AbstractionRRUnpacker(MockUnpacker):
-    targets = [AbstractionRR]
+    targets = (AbstractionRR,)

pytest_ofrak/src/pytest_ofrak/patterns/basic_block_unpacker.py

@@ -7,7 +7,7 @@
 """
 import os
 from dataclasses import dataclass
-from typing import Dict, List, Union, Tuple
+from typing import Dict, List, Set, Union, Tuple
 
 import pytest
 from ofrak.core.filesystem import File
@@ -37,9 +37,13 @@ class BasicBlockUnpackerTestCase(
     binary_filename: str
     binary_md5_digest: str
     basic_block_data_ranges_in_root: Dict[int, Range]  # Used when created basic blocks manually
+    # Workaround for https://github.com/python/mypy/issues/12633, fixed in mypy 0.950.
+    # Remove these two lines once using a newer mypy.
+    expected_results: Dict[int, List[ExpectedBasicBlockUnpackResult]]
+    optional_results: Set[int]
 
 
-BASIC_BLOCK_UNPACKER_TEST_CASES = [
+BASIC_BLOCK_UNPACKER_TEST_CASES: List[BasicBlockUnpackerTestCase] = [
     BasicBlockUnpackerTestCase(
         "x64",
         {
@@ -1582,10 +1586,11 @@ async def unpack_verify_test_case(self, request) -> BasicBlockUnpackerTestCase:
     @pytest.fixture
     async def root_resource(
         self,
-        unpack_verify_test_case: BasicBlockUnpackerTestCase,
+        unpack_verify_test_case: UnpackAndVerifyTestCase,
         ofrak_context: OFRAKContext,
         test_id: str,
     ) -> Resource:
+        assert isinstance(unpack_verify_test_case, BasicBlockUnpackerTestCase)
         asset_path = os.path.join(ASSETS_DIR, unpack_verify_test_case.binary_filename)
         with open(asset_path, "rb") as f:
             binary_data = f.read()
@@ -1598,9 +1603,9 @@ async def verify_descendant(
         instructions = await basic_block.get_instructions()
 
         # Check that the parent complex blocks are extracted as expected
-        instructions_by_addr: Dict[int, ExpectedBasicBlockUnpackResult] = dict()
+        instructions_by_addr: Dict[int, Tuple[Instruction, ...]] = {}
         for expected_instructions in specified_result:
-            if type(expected_instructions) is tuple:
+            if isinstance(expected_instructions, tuple):
                 instructions_by_addr[
                     expected_instructions[0].virtual_address
                 ] = expected_instructions
@@ -1654,7 +1659,7 @@ async def verify_descendant(
         if len(errors) > 0:
             raise ValueError(*errors)
 
-    async def get_descendants_to_verify(self, unpacked_resource: Resource) -> Dict[int, Resource]:
+    async def get_descendants_to_verify(self, unpacked_resource: Resource) -> Dict[int, BasicBlock]:
         elf = await unpacked_resource.view_as(Elf)
         text_section = await elf.get_section_by_name(".text")
         basic_blocks: List[BasicBlock] = list(

pytest_ofrak/src/pytest_ofrak/patterns/code_region_unpacker.py

@@ -7,7 +7,7 @@
 """
 import os.path
 from dataclasses import dataclass
-from typing import List, Dict
+from typing import Dict, List, Set
 
 import pytest
 
@@ -30,9 +30,13 @@
 class CodeRegionUnpackerTestCase(UnpackAndVerifyTestCase[int, List[int]]):
     binary_filename: str
     binary_md5_digest: str
+    # Workaround for https://github.com/python/mypy/issues/12633, fixed in mypy 0.950.
+    # Remove these two lines once using a newer mypy.
+    expected_results: Dict[int, List[int]]
+    optional_results: Set[int]
 
 
-CODE_REGION_UNPACKER_TEST_CASES = [
+CODE_REGION_UNPACKER_TEST_CASES: List[CodeRegionUnpackerTestCase] = [
     CodeRegionUnpackerTestCase(
         "x64",
         {
@@ -147,10 +151,11 @@ async def unpack_verify_test_case(self, request) -> CodeRegionUnpackerTestCase:
     @pytest.fixture
     async def root_resource(
         self,
-        unpack_verify_test_case: CodeRegionUnpackerTestCase,
+        unpack_verify_test_case: UnpackAndVerifyTestCase,
         ofrak_context: OFRAKContext,
         test_id: str,
     ) -> Resource:
+        assert isinstance(unpack_verify_test_case, CodeRegionUnpackerTestCase)
         asset_path = os.path.join(ASSETS_DIR, unpack_verify_test_case.binary_filename)
         with open(asset_path, "rb") as f:
             binary_data = f.read()
@@ -169,7 +174,9 @@ async def unpack(self, root_resource: Resource):
         """
         await root_resource.unpack_recursively(do_not_unpack=(BasicBlock,))
 
-    async def get_descendants_to_verify(self, unpacked_resource: Resource) -> Dict[int, Resource]:
+    async def get_descendants_to_verify(
+        self, unpacked_resource: Resource
+    ) -> Dict[int, ComplexBlock]:
         program = await unpacked_resource.view_as(Program)
         code_regions = await program.get_code_regions()