Add mypy ignore to config, and remove ignore comments in existing tes…

…ts. (#1317)
ThePalaceProject · Aug 9, 2023 · 00a3e36 · 00a3e36
1 parent c99b01c
commit 00a3e36
Show file tree

Hide file tree

Showing 24 changed files with 102 additions and 95 deletions.
diff --git a/pyproject.toml b/pyproject.toml
@@ -45,6 +45,15 @@ warn_unreachable = true
 warn_unused_configs = true
 warn_unused_ignores = true
 
+[[tool.mypy.overrides]]
+# In our tests, we often overwrite methods on classes to mock out behavior.
+# This is a common pattern in Python, but mypy doesn't like it. This override
+# silences those errors, but only for the tests module.
+# See discussion here:
+# https://github.com/python/mypy/issues/2427
+disable_error_code = "method-assign"
+module = "tests.*"
+
 [[tool.mypy.overrides]]
 # This override is the equivalent of running mypy with the --strict flag.
 # This is a work in progress, but we should try to get as many of our files

diff --git a/tests/api/admin/controller/test_library.py b/tests/api/admin/controller/test_library.py
@@ -679,8 +679,8 @@ def test_library_delete(self, settings_ctrl_fixture):
     def test_process_libraries(self, controller_fixture: ControllerFixture):
         manager = MagicMock()
         controller = LibrarySettingsController(manager)
-        controller.process_get = MagicMock()  # type: ignore[method-assign]
-        controller.process_post = MagicMock()  # type: ignore[method-assign]
+        controller.process_get = MagicMock()
+        controller.process_post = MagicMock()
 
         # Make sure we call process_get for a get request
         with controller_fixture.request_context_with_library("/", method="GET"):

diff --git a/tests/api/saml/configuration/test_model.py b/tests/api/saml/configuration/test_model.py
@@ -68,7 +68,7 @@ def test_get_service_provider_returns_correct_value(
     ):
         # Arrange
         metadata_parser = SAMLMetadataParser()
-        metadata_parser.parse = MagicMock(side_effect=metadata_parser.parse)  # type: ignore[method-assign]
+        metadata_parser.parse = MagicMock(side_effect=metadata_parser.parse)
 
         configuration = create_saml_configuration(
             service_provider_xml_metadata=saml_strings.CORRECT_XML_WITH_ONE_SP
@@ -94,7 +94,7 @@ def test_get_identity_providers_returns_non_federated_idps(
     ):
         # Arrange
         metadata_parser = SAMLMetadataParser()
-        metadata_parser.parse = MagicMock(side_effect=metadata_parser.parse)  # type: ignore[method-assign]
+        metadata_parser.parse = MagicMock(side_effect=metadata_parser.parse)
         configuration = create_saml_configuration(
             non_federated_identity_provider_xml_metadata=saml_strings.CORRECT_XML_WITH_MULTIPLE_IDPS
         )
@@ -179,7 +179,7 @@ def test_get_identity_providers_returns_both_non_federated_and_federated_idps(
         ]
 
         metadata_parser = SAMLMetadataParser()
-        metadata_parser.parse = MagicMock(side_effect=metadata_parser.parse)  # type: ignore[method-assign]
+        metadata_parser.parse = MagicMock(side_effect=metadata_parser.parse)
 
         federation = SAMLFederation("Test federation", "http://localhost")
         federated_idp_1 = SAMLFederatedIdentityProvider(

diff --git a/tests/api/saml/conftest.py b/tests/api/saml/conftest.py
@@ -63,10 +63,10 @@ def _create_mock(
         if configuration is None:
             configuration = create_saml_configuration()
         onelogin_configuration = SAMLOneLoginConfiguration(configuration)
-        onelogin_configuration._load_identity_providers = MagicMock(  # type: ignore[method-assign]
+        onelogin_configuration._load_identity_providers = MagicMock(
             return_value=identity_providers
         )
-        onelogin_configuration._load_service_provider = MagicMock(  # type: ignore[method-assign]
+        onelogin_configuration._load_service_provider = MagicMock(
             return_value=service_provider
         )
         return onelogin_configuration

diff --git a/tests/api/saml/test_controller.py b/tests/api/saml/test_controller.py
@@ -415,7 +415,7 @@ def test_saml_authentication_callback(
         authenticator.library_authenticators[
             "default"
         ].bearer_token_signing_secret = "test"
-        authenticator.create_bearer_token = MagicMock(return_value=bearer_token)  # type: ignore[method-assign]
+        authenticator.create_bearer_token = MagicMock(return_value=bearer_token)
 
         controller = SAMLController(controller_fixture.app.manager, authenticator)
 

diff --git a/tests/api/test_authenticator.py b/tests/api/test_authenticator.py
@@ -864,8 +864,10 @@ def test_authenticated_patron_basic(
             neighborhood="Achewood",
         )
         basic = mock_basic(patrondata=patrondata)
-        basic.authenticate = MagicMock(return_value=patron)  # type: ignore[method-assign]
-        basic.integration = PropertyMock(return_value=MagicMock(spec=IntegrationConfiguration))  # type: ignore[method-assign]
+        basic.authenticate = MagicMock(return_value=patron)
+        basic.integration = PropertyMock(
+            return_value=MagicMock(spec=IntegrationConfiguration)
+        )
         authenticator = LibraryAuthenticator(
             _db=db.session,
             library=db.default_library(),
@@ -1480,11 +1482,11 @@ def test_authenticated_patron_only_calls_remote_patron_lookup_once(
         # more than once. This test makes sure that if we have a complete patrondata from remote_authenticate,
         # or from enforce_library_identifier_restriction, we don't call remote_patron_lookup.
         provider = mock_basic()
-        provider.remote_authenticate = MagicMock(return_value=auth_return)  # type: ignore[method-assign]
-        provider.enforce_library_identifier_restriction = MagicMock(  # type: ignore[method-assign]
+        provider.remote_authenticate = MagicMock(return_value=auth_return)
+        provider.enforce_library_identifier_restriction = MagicMock(
             return_value=enforce_return
         )
-        provider.remote_patron_lookup = MagicMock(return_value=lookup_return)  # type: ignore[method-assign]
+        provider.remote_patron_lookup = MagicMock(return_value=lookup_return)
 
         username = "a"
         password = "b"
@@ -1827,7 +1829,7 @@ def test_enforce_library_identifier_restriction(
         remote_patrondata = PatronData(
             library_identifier=identifier, authorization_identifier="123"
         )
-        provider.remote_patron_lookup = MagicMock(return_value=remote_patrondata)  # type: ignore[method-assign]
+        provider.remote_patron_lookup = MagicMock(return_value=remote_patrondata)
         if expected:
             assert (
                 provider.enforce_library_identifier_restriction(local_patrondata)
@@ -1955,7 +1957,7 @@ def test_testing_patron(
                 test_password="2",
             )
         )
-        missing_patron.authenticated_patron = MagicMock(return_value=None)  # type: ignore[method-assign]
+        missing_patron.authenticated_patron = MagicMock(return_value=None)
         value = missing_patron.testing_patron(db.session)
         assert (None, "2") == value
         missing_patron.authenticated_patron.assert_called_once()
@@ -1975,7 +1977,9 @@ def test_testing_patron(
                 test_password="2",
             )
         )
-        problem_patron.authenticated_patron = MagicMock(return_value=PATRON_OF_ANOTHER_LIBRARY)  # type: ignore[method-assign]
+        problem_patron.authenticated_patron = MagicMock(
+            return_value=PATRON_OF_ANOTHER_LIBRARY
+        )
         value = problem_patron.testing_patron(db.session)
         assert (PATRON_OF_ANOTHER_LIBRARY, "2") == value
 
@@ -1990,7 +1994,7 @@ def test_testing_patron(
         # results in something (non None) that's not a Patron
         # or a problem detail document.
         not_a_patron = "<not a patron>"
-        problem_patron.authenticated_patron = MagicMock(return_value=not_a_patron)  # type: ignore[method-assign]
+        problem_patron.authenticated_patron = MagicMock(return_value=not_a_patron)
         value = problem_patron.testing_patron(db.session)
         assert (not_a_patron, "2") == value
 
@@ -2010,7 +2014,7 @@ def test_testing_patron(
                 test_password="2",
             )
         )
-        present_patron.authenticated_patron = MagicMock(return_value=patron)  # type: ignore[method-assign]
+        present_patron.authenticated_patron = MagicMock(return_value=patron)
         value = present_patron.testing_patron(db.session)
         assert (patron, "2") == value
 
@@ -2025,7 +2029,7 @@ def test__run_self_tests(self, mock_basic: MockBasicFixture):
         # aren't even run.
         provider = mock_basic()
         exception = Exception("Nope")
-        provider.testing_patron_or_bust = MagicMock(side_effect=exception)  # type: ignore[method-assign]
+        provider.testing_patron_or_bust = MagicMock(side_effect=exception)
         [result] = list(provider._run_self_tests(_db))
         provider.testing_patron_or_bust.assert_called_once_with(_db)
         assert result.success is False
@@ -2034,8 +2038,8 @@ def test__run_self_tests(self, mock_basic: MockBasicFixture):
         # If we can authenticate a test patron, the patron and their
         # password are passed into the next test.
         provider = mock_basic()
-        provider.testing_patron_or_bust = MagicMock(return_value=("patron", "password"))  # type: ignore[method-assign]
-        provider.update_patron_metadata = MagicMock(return_value="some metadata")  # type: ignore[method-assign]
+        provider.testing_patron_or_bust = MagicMock(return_value=("patron", "password"))
+        provider.update_patron_metadata = MagicMock(return_value="some metadata")
 
         [get_patron, update_metadata] = provider._run_self_tests(_db)
         provider.testing_patron_or_bust.assert_called_once_with(_db)
@@ -2308,7 +2312,9 @@ def test_success_with_immediate_patron_sync(
         # set of information. If a remote patron lookup were to happen,
         # it would explode.
         provider = mock_basic(patrondata=complete_patrondata)
-        provider.remote_patron_lookup = MagicMock(side_effect=Exception("Should not be called."))  # type: ignore[method-assign]
+        provider.remote_patron_lookup = MagicMock(
+            side_effect=Exception("Should not be called.")
+        )
 
         # The patron can be authenticated.
         assert patron == provider.authenticate(db.session, self.credentials)

diff --git a/tests/api/test_circulationapi.py b/tests/api/test_circulationapi.py
@@ -190,7 +190,7 @@ def test_borrowing_of_unlimited_access_book_succeeds(
 
         # Reset the API map, this book belongs to the "basic" collection,
         # i.e. collection without a custom CirculationAPI implementation.
-        circulation_api.circulation.api_for_license_pool = MagicMock(return_value=None)  # type: ignore[method-assign]
+        circulation_api.circulation.api_for_license_pool = MagicMock(return_value=None)
 
         # Mark the book as unlimited access.
         circulation_api.pool.unlimited_access = True
@@ -1043,7 +1043,7 @@ def test_fulfilment_of_unlimited_access_book_succeeds(
         are fulfilled in the same way as OA and self-hosted books."""
         # Reset the API map, this book belongs to the "basic" collection,
         # i.e. collection without a custom CirculationAPI implementation.
-        circulation_api.circulation.api_for_license_pool = MagicMock(return_value=None)  # type: ignore[method-assign]
+        circulation_api.circulation.api_for_license_pool = MagicMock(return_value=None)
 
         # Mark the book as unlimited access.
         circulation_api.pool.unlimited_access = True
@@ -1132,7 +1132,7 @@ def try_to_fulfill():
         def yes_we_can(*args, **kwargs):
             return True
 
-        circulation_api.circulation.can_fulfill_without_loan = yes_we_can  # type: ignore[method-assign]
+        circulation_api.circulation.can_fulfill_without_loan = yes_we_can
         result = try_to_fulfill()
         assert fulfillment == result
 

diff --git a/tests/api/test_controller_base.py b/tests/api/test_controller_base.py
@@ -67,7 +67,7 @@ def set_patron():
         mock = MagicMock(
             side_effect=set_patron, return_value="return value will be ignored"
         )
-        circulation_fixture.controller.authenticated_patron_from_request = mock  # type: ignore[method-assign]
+        circulation_fixture.controller.authenticated_patron_from_request = mock
         with circulation_fixture.app.test_request_context("/"):
             assert o2 == circulation_fixture.controller.request_patron
 
@@ -692,7 +692,7 @@ def test_load_lane(self, circulation_fixture: CirculationControllerFixture):
 
         # Mock Lane.accessible_to so that it always returns
         # false.
-        lane.accessible_to = MagicMock(return_value=False)  # type: ignore[method-assign]
+        lane.accessible_to = MagicMock(return_value=False)
         headers = dict(Authorization=circulation_fixture.valid_auth)
         with circulation_fixture.request_context_with_library(
             "/", headers=headers, library=circulation_fixture.db.default_library()

diff --git a/tests/api/test_controller_cm.py b/tests/api/test_controller_cm.py
@@ -57,7 +57,7 @@ def mock_for_library(incoming_library):
             return None
 
         old_for_library = CustomIndexView.for_library
-        CustomIndexView.for_library = mock_for_library  # type: ignore[method-assign]
+        CustomIndexView.for_library = mock_for_library
 
         # We also set up some configuration settings that will
         # be loaded.
@@ -140,7 +140,7 @@ def mock_for_library(incoming_library):
         } == manager.patron_web_domains
 
         # Restore the CustomIndexView.for_library implementation
-        CustomIndexView.for_library = old_for_library  # type: ignore[method-assign]
+        CustomIndexView.for_library = old_for_library
 
     def test_exception_during_external_search_initialization_is_stored(
         self, circulation_fixture: CirculationControllerFixture

diff --git a/tests/api/test_controller_crawlfeed.py b/tests/api/test_controller_crawlfeed.py
@@ -41,9 +41,9 @@ def mock(title, url, worklist, annotator=None, feed_class=AcquisitionFeed):
             )
             return "An OPDS feed."
 
-        controller._crawlable_feed = mock  # type: ignore[method-assign]
+        controller._crawlable_feed = mock
         yield
-        controller._crawlable_feed = original  # type: ignore[method-assign]
+        controller._crawlable_feed = original
 
     def test_crawlable_library_feed(
         self, circulation_fixture: CirculationControllerFixture

diff --git a/tests/api/test_controller_loan.py b/tests/api/test_controller_loan.py
@@ -833,13 +833,13 @@ def test_fulfill_without_active_loan(self, loan_fixture: LoanFixture):
         def mock_authenticated_patron():
             return INTEGRATION_ERROR
 
-        controller.authenticated_patron_from_request = mock_authenticated_patron  # type: ignore[method-assign]
+        controller.authenticated_patron_from_request = mock_authenticated_patron
         with loan_fixture.request_context_with_library("/"):
             problem = controller.fulfill(
                 loan_fixture.pool.id, loan_fixture.mech2.delivery_mechanism.id
             )
             assert INTEGRATION_ERROR == problem
-        controller.authenticated_patron_from_request = old_authenticated_patron  # type: ignore[method-assign]
+        controller.authenticated_patron_from_request = old_authenticated_patron
 
         # However, if can_fulfill_without_loan returns True, then
         # fulfill() will be called. If fulfill() returns a
@@ -865,7 +865,7 @@ def mock_fulfill(*args, **kwargs):
         # Now we're able to fulfill the book even without
         # authenticating a patron.
         with loan_fixture.request_context_with_library("/"):
-            controller.can_fulfill_without_loan = mock_can_fulfill_without_loan  # type: ignore[method-assign]
+            controller.can_fulfill_without_loan = mock_can_fulfill_without_loan
             controller.circulation.fulfill = mock_fulfill
             response = controller.fulfill(
                 loan_fixture.pool.id, loan_fixture.mech2.delivery_mechanism.id
@@ -940,8 +940,8 @@ def test_no_drm_fulfill(self, loan_fixture: LoanFixture):
             None,
             content_link_redirect=True,
         )
-        controller.can_fulfill_without_loan = MagicMock(return_value=False)  # type: ignore[method-assign]
-        controller.authenticated_patron_from_request = MagicMock(return_value=patron)  # type: ignore[method-assign]
+        controller.can_fulfill_without_loan = MagicMock(return_value=False)
+        controller.authenticated_patron_from_request = MagicMock(return_value=patron)
 
         with loan_fixture.request_context_with_library(
             "/",
@@ -1137,7 +1137,7 @@ def handle_conditional_request(last_modified=None):
         original_handle_conditional_request = (
             loan_fixture.controller.handle_conditional_request
         )
-        loan_fixture.manager.loans.handle_conditional_request = (  # type: ignore[method-assign]
+        loan_fixture.manager.loans.handle_conditional_request = (
             handle_conditional_request
         )
 
@@ -1176,7 +1176,7 @@ def handle_conditional_request(last_modified=None):
         # the course of this test, but it will not notice any more
         # conditional requests -- the detailed behavior of
         # handle_conditional_request is tested elsewhere.
-        loan_fixture.manager.loans.handle_conditional_request = (  # type: ignore[method-assign]
+        loan_fixture.manager.loans.handle_conditional_request = (
             original_handle_conditional_request
         )
 

diff --git a/tests/api/test_controller_opdsfeed.py b/tests/api/test_controller_opdsfeed.py
@@ -775,7 +775,7 @@ def test_qa_feed(self, circulation_fixture: CirculationControllerFixture):
         # JackpotWorkList and passes it into _qa_feed.
 
         mock = MagicMock(return_value="an OPDS feed")
-        circulation_fixture.manager.opds_feeds._qa_feed = mock  # type: ignore[method-assign]
+        circulation_fixture.manager.opds_feeds._qa_feed = mock
 
         response = circulation_fixture.manager.opds_feeds.qa_feed()
         [call] = mock.mock_calls
@@ -814,7 +814,7 @@ def test_qa_feed2(self, circulation_fixture: CirculationControllerFixture):
         # JackpotWorkList and passes it into _qa_feed.
 
         mock = MagicMock(return_value="an OPDS feed")
-        circulation_fixture.manager.opds_feeds._qa_feed = mock  # type: ignore[method-assign]
+        circulation_fixture.manager.opds_feeds._qa_feed = mock
 
         response = circulation_fixture.manager.opds_feeds.qa_feed()
         [call] = mock.mock_calls
@@ -854,7 +854,7 @@ def test_qa_series_feed(self, circulation_fixture: CirculationControllerFixture)
         # instructions to use HasSeriesFacets.
 
         mock = MagicMock(return_value="an OPDS feed")
-        circulation_fixture.manager.opds_feeds._qa_feed = mock  # type: ignore[method-assign]
+        circulation_fixture.manager.opds_feeds._qa_feed = mock
 
         response = circulation_fixture.manager.opds_feeds.qa_series_feed()
         [call] = mock.mock_calls

diff --git a/tests/api/test_novelist.py b/tests/api/test_novelist.py
@@ -701,15 +701,15 @@ def mockHTTPPut(url, headers, **kwargs):
             return MockRequestsResponse(200, content=json.dumps(mock_response))
 
         oldPut = novelist_fixture.novelist.put
-        novelist_fixture.novelist.put = mockHTTPPut  # type: ignore[method-assign]
+        novelist_fixture.novelist.put = mockHTTPPut
 
         response = novelist_fixture.novelist.put_items_novelist(
             novelist_fixture.db.default_library()
         )
 
         assert response == mock_response
 
-        novelist_fixture.novelist.put = oldPut  # type: ignore[method-assign]
+        novelist_fixture.novelist.put = oldPut
 
     def test_make_novelist_data_object(self, novelist_fixture: NoveListFixture):
         bad_data = []  # type: ignore

diff --git a/tests/api/test_odilo.py b/tests/api/test_odilo.py
@@ -313,7 +313,7 @@ def test_run_self_tests_short_circuit(self, odilo: OdiloFixture):
         def explode(*args, **kwargs):
             raise Exception("Failure!")
 
-        odilo.api.check_creds = explode  # type: ignore[method-assign]
+        odilo.api.check_creds = explode
 
         # Only one test will be run.
         [check_creds] = odilo.api._run_self_tests(odilo.db.session)

diff --git a/tests/api/test_opds2.py b/tests/api/test_opds2.py
@@ -326,7 +326,7 @@ def test_opds2_with_authentication_tokens(
         loan = loans.first()
         assert isinstance(loan, Loan)
         mechanism_id = loan.license_pool.delivery_mechanisms[0].delivery_mechanism.id
-        manager.loans.authenticated_patron_from_request = lambda: patron  # type: ignore[method-assign]
+        manager.loans.authenticated_patron_from_request = lambda: patron
 
         # Fulfill (Download) the book, should redirect to an authenticated URL
         with controller_fixture.request_context_with_library("/") as ctx, patch.object(