From b5e572811257a33a25494af34a5231823cbe5f02 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Mon, 29 May 2023 15:01:44 -0700 Subject: [PATCH] coverage: Clean up coverage configuration. Signed-off-by: Anders Kaseorg --- corporate/views/portico.py | 2 +- tools/coveragerc | 34 ++++---------- tools/test-backend | 58 ++++++++++-------------- zerver/openapi/openapi.py | 4 +- zerver/views/development/cache.py | 2 +- zerver/views/development/camo.py | 4 +- zerver/views/development/dev_login.py | 2 +- zerver/views/development/integrations.py | 6 +-- zerver/views/development/registration.py | 2 +- zproject/config.py | 4 +- zproject/configured_settings.py | 2 +- zproject/default_settings.py | 2 +- zproject/dev_urls.py | 2 +- zproject/urls.py | 8 ++-- 14 files changed, 53 insertions(+), 79 deletions(-) diff --git a/corporate/views/portico.py b/corporate/views/portico.py index cd435f3d64..b05b705894 100644 --- a/corporate/views/portico.py +++ b/corporate/views/portico.py @@ -152,7 +152,7 @@ def communities_view(request: HttpRequest) -> HttpResponse: org_types.pop("unspecified", None) # Change display name of non-profit orgs. - if org_types.get("nonprofit"): + if org_types.get("nonprofit"): # nocoverage org_types["nonprofit"]["name"] = "Non-profit" return TemplateResponse( diff --git a/tools/coveragerc b/tools/coveragerc index a31c7929d7..190f6cbbb3 100644 --- a/tools/coveragerc +++ b/tools/coveragerc @@ -6,7 +6,7 @@ exclude_lines = # Don't complain if non-runnable code isn't run: if False: # Don't require coverage for base class NotImplementedErrors - raise NotImplementedError() + raise NotImplementedError # Don't require coverage for test suite AssertionError -- they're usually for clarity raise AssertionError # Don't require coverage for __str__ statements just used for printing @@ -31,27 +31,11 @@ data_file=var/.coverage # overhead but is very useful for finding existing tests for a code path. dynamic_context=test_function -omit = - */zulip-venv-cache/* - */migrations/* - */management/commands/* - # Parts of the test runner infrastructure - tools/test-backend - zerver/lib/test_fixtures.py - zerver/lib/test_runner.py - # Has its own independent test suite - zerver/openapi/python_examples.py - # Debugging tools that don't lend themselves well to unit tests - zerver/lib/debug.py - # Part of provisioning/populate_db - zerver/lib/generate_test_data.py - # Excluded because its coverage state is flaky. - zerver/tornado/ioloop_logging.py - # Zulip's library for use in scripts - scripts/lib/zulip_tools.py - # Used only for the legacy Zephyr integration, and unlikely to ever be unit-tested - zerver/lib/ccache.py - # Settings.py files are hard to test - zproject/*settings.py - # https://github.com/davidhalter/jedi/issues/1122 - blub +source = + analytics/ + confirmation/ + corporate/ + pgroonga/ + zerver/ + zilencer/ + zproject/ diff --git a/tools/test-backend b/tools/test-backend index 00823a56f8..61aa604cbf 100755 --- a/tools/test-backend +++ b/tools/test-backend @@ -31,39 +31,19 @@ import responses from django.conf import settings from django.test.utils import get_runner -target_fully_covered = [ - "analytics/lib/*.py", - "analytics/models.py", - "analytics/tests/*.py", - "analytics/views/*.py", - # zerver/ and zerver/lib/ are important core files - "zerver/*.py", - "zerver/actions/*.py", - "zerver/lib/*.py", - "zerver/lib/*/*.py", - "zerver/lib/*/*/*.py", - "zerver/data_import/*.py", - "zerver/templatetags/*.py", - "zerver/tornado/*.py", - # Billing files require 100% test coverage - "corporate/lib/stripe.py", - "corporate/views.py", - # Test files should have 100% coverage; test code that isn't run - # is likely a bug in the test. - "zerver/tests/*.py", - "corporate/tests/*.py", - # As a project, we require 100% test coverage in the views files. - "zerver/views/*.py", - "zproject/backends.py", - "confirmation/*.py", - "zerver/webhooks/*/*.py", - # Once we have a nice negative tests system, we can add these: - # 'zerver/webhooks/*/*.py', - # 'zerver/webhooks/*/*/*.py', - "zerver/worker/*.py", +source_files = [ + "analytics/**/*.py", + "confirmation/**/*.py", + "corporate/**/*.py", + "pgroonga/**/*.py", + "zerver/**/*.py", + "zilencer/**/*.py", + "zproject/**/*.py", ] not_yet_fully_covered = [ + "*/migrations/*.py", + "*/management/commands/*.py", # Analytics fixtures library is used to generate test fixtures; # isn't properly accounted for in test coverage analysis since it # runs before tests. @@ -100,14 +80,12 @@ not_yet_fully_covered = [ "zerver/lib/fix_unreads.py", "zerver/lib/import_realm.py", "zerver/lib/logging_util.py", - "zerver/lib/migrate.py", "zerver/lib/profile.py", "zerver/lib/queue.py", "zerver/lib/sqlalchemy_utils.py", "zerver/lib/storage.py", "zerver/lib/zephyr.py", "zerver/lib/templates.py", - "zerver/templatetags/minified_js.py", # Low priority for coverage "zerver/lib/ccache.py", "zerver/lib/generate_test_data.py", @@ -115,7 +93,10 @@ not_yet_fully_covered = [ "zerver/lib/test_fixtures.py", "zerver/lib/test_runner.py", "zerver/lib/test_console_output.py", + "zerver/openapi/curl_param_value_generators.py", + "zerver/openapi/javascript_examples.py", "zerver/openapi/python_examples.py", + "zerver/openapi/test_curl_examples.py", # Tornado should ideally have full coverage, but we're not there. "zerver/tornado/descriptors.py", "zerver/tornado/django_api.py", @@ -142,11 +123,18 @@ not_yet_fully_covered = [ # Cannot have coverage, as tests run in a transaction "zerver/lib/safe_session_cached_db.py", "zerver/lib/singleton_bmemcached.py", + # Branches in Django settings files are hard to test + "zproject/computed_settings.py", + "zproject/dev_settings.py", + "zproject/test_extra_settings.py", + # Better tested in a full deployment + "zproject/sentry.py", + "zproject/wsgi.py", ] enforce_fully_covered = sorted( - {path for target in target_fully_covered for path in glob.glob(target)} - - {path for target in not_yet_fully_covered for path in glob.glob(target)} + {path for target in source_files for path in glob.glob(target, recursive=True)} + - {path for target in not_yet_fully_covered for path in glob.glob(target, recursive=True)} ) FAILED_TEST_PATH = "var/last_test_failure.json" @@ -473,7 +461,7 @@ def main() -> None: for path in not_yet_fully_covered: try: missing_lines = cov.analysis2(path)[3] - if len(missing_lines) == 0 and path != "zerver/lib/migrate.py": + if not missing_lines: print( f"ERROR: {path} has complete backend test coverage but is still in not_yet_fully_covered." ) diff --git a/zerver/openapi/openapi.py b/zerver/openapi/openapi.py index 09025bc80c..91bad649e2 100644 --- a/zerver/openapi/openapi.py +++ b/zerver/openapi/openapi.py @@ -386,12 +386,12 @@ def validate_against_openapi_schema( if (endpoint, method) in EXCLUDE_UNDOCUMENTED_ENDPOINTS: return False # Return true for endpoints with only response documentation remaining - if (endpoint, method) in EXCLUDE_DOCUMENTED_ENDPOINTS: + if (endpoint, method) in EXCLUDE_DOCUMENTED_ENDPOINTS: # nocoverage return True # Check if the response matches its code if status_code.startswith("2") and ( content.get("result", "success").lower() not in ["success", "partially_completed"] - ): + ): # nocoverage raise SchemaError("Response is not 200 but is validating against 200 schema") # Code is not declared but appears in various 400 responses. If # common, it can be added to 400 response schema diff --git a/zerver/views/development/cache.py b/zerver/views/development/cache.py index cbabdddb0a..e1b87c7724 100644 --- a/zerver/views/development/cache.py +++ b/zerver/views/development/cache.py @@ -14,7 +14,7 @@ ZULIP_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), "../../../ # This is used only by the Puppeteer tests to clear all the cache after each run. @csrf_exempt @require_post -def remove_caches(request: HttpRequest) -> HttpResponse: +def remove_caches(request: HttpRequest) -> HttpResponse: # nocoverage cache = get_cache_backend(None) cache.clear() clear_client_cache() diff --git a/zerver/views/development/camo.py b/zerver/views/development/camo.py index 86189fcae8..07f86f8f18 100644 --- a/zerver/views/development/camo.py +++ b/zerver/views/development/camo.py @@ -5,7 +5,9 @@ from zerver.lib.camo import is_camo_url_valid from zerver.lib.thumbnail import generate_thumbnail_url -def handle_camo_url(request: HttpRequest, digest: str, received_url: str) -> HttpResponse: +def handle_camo_url( + request: HttpRequest, digest: str, received_url: str +) -> HttpResponse: # nocoverage original_url = bytes.fromhex(received_url).decode() if is_camo_url_valid(digest, original_url): return redirect(generate_thumbnail_url(original_url)) diff --git a/zerver/views/development/dev_login.py b/zerver/views/development/dev_login.py index f42f53a35e..55213dc1bd 100644 --- a/zerver/views/development/dev_login.py +++ b/zerver/views/development/dev_login.py @@ -124,7 +124,7 @@ def api_dev_fetch_api_key(request: HttpRequest, username: str = REQ()) -> HttpRe raise RealmDeactivatedError if return_data.get("inactive_user"): raise UserDeactivatedError - if return_data.get("invalid_subdomain"): + if return_data.get("invalid_subdomain"): # nocoverage raise InvalidSubdomainError if user_profile is None: # Since we're not actually checking passwords, this condition diff --git a/zerver/views/development/integrations.py b/zerver/views/development/integrations.py index 96dd7a5431..8c69aa61e5 100644 --- a/zerver/views/development/integrations.py +++ b/zerver/views/development/integrations.py @@ -93,7 +93,7 @@ def get_fixtures(request: HttpRequest, integration_name: str = REQ()) -> HttpRes valid_integration_name, "".join(fixture.split(".")[:-1]) ) - def fix_name(header: str) -> str: + def fix_name(header: str) -> str: # nocoverage if header.startswith("HTTP_"): # HTTP_ is a prefix intended for Django. return header[len("HTTP_") :] return header @@ -114,7 +114,7 @@ def check_send_webhook_fixture_message( ) -> HttpResponseBase: try: custom_headers_dict = orjson.loads(custom_headers) - except orjson.JSONDecodeError as ve: + except orjson.JSONDecodeError as ve: # nocoverage raise JsonableError(f"Custom HTTP headers are not in a valid JSON format. {ve}") # nolint response = send_webhook_fixture_message(url, body, is_json, custom_headers_dict) @@ -130,7 +130,7 @@ def send_all_webhook_fixture_messages( request: HttpRequest, url: str = REQ(), integration_name: str = REQ() ) -> HttpResponse: valid_integration_name = get_valid_integration_name(integration_name) - if not valid_integration_name: + if not valid_integration_name: # nocoverage raise ResourceNotFoundError(f'"{integration_name}" is not a valid webhook integration.') fixtures_dir = os.path.join(ZULIP_PATH, f"zerver/webhooks/{valid_integration_name}/fixtures") diff --git a/zerver/views/development/registration.py b/zerver/views/development/registration.py index e4750eab07..44f3323b60 100644 --- a/zerver/views/development/registration.py +++ b/zerver/views/development/registration.py @@ -40,7 +40,7 @@ def generate_demo_realm_name() -> str: @csrf_exempt def register_development_user(request: HttpRequest) -> HttpResponse: realm = get_realm_from_request(request) - if realm is None: + if realm is None: # nocoverage return HttpResponseRedirect( f"{settings.EXTERNAL_URI_SCHEME}{settings.REALM_HOSTS['zulip']}/devtools/register_user/", status=307, diff --git a/zproject/config.py b/zproject/config.py index ba56ccbd11..e6765aeb41 100644 --- a/zproject/config.py +++ b/zproject/config.py @@ -16,7 +16,7 @@ config_file.read("/etc/zulip/zulip.conf") PRODUCTION = config_file.has_option("machine", "deploy_type") DEVELOPMENT = not PRODUCTION secrets_file = configparser.RawConfigParser() -if PRODUCTION: +if PRODUCTION: # nocoverage secrets_file.read("/etc/zulip/zulip-secrets.conf") else: secrets_file.read(os.path.join(DEPLOY_ROOT, "zproject/dev-secrets.conf")) @@ -37,7 +37,7 @@ def get_secret( def get_secret( key: str, default_value: Optional[str] = None, development_only: bool = False ) -> Optional[str]: - if development_only and PRODUCTION: + if development_only and PRODUCTION: # nocoverage return default_value return secrets_file.get("secrets", key, fallback=default_value) diff --git a/zproject/configured_settings.py b/zproject/configured_settings.py index aab10b9c15..8e920cfcfe 100644 --- a/zproject/configured_settings.py +++ b/zproject/configured_settings.py @@ -11,7 +11,7 @@ from .default_settings import * # noqa: F403 isort: skip # Import prod_settings after determining the deployment/machine type from .config import PRODUCTION -if PRODUCTION: +if PRODUCTION: # nocoverage from .prod_settings import * # noqa: F403 isort: skip else: # For the Dev VM environment, we use the same settings as the diff --git a/zproject/default_settings.py b/zproject/default_settings.py index 170c6e83f0..9941671964 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -10,7 +10,7 @@ from .config import DEVELOPMENT, PRODUCTION, get_secret if TYPE_CHECKING: from django_auth_ldap.config import LDAPSearch -if PRODUCTION: +if PRODUCTION: # nocoverage from .prod_settings import EXTERNAL_HOST, ZULIP_ADMINISTRATOR else: from .dev_settings import EXTERNAL_HOST, ZULIP_ADMINISTRATOR diff --git a/zproject/dev_urls.py b/zproject/dev_urls.py index 281250fb60..cb0d8fbb97 100644 --- a/zproject/dev_urls.py +++ b/zproject/dev_urls.py @@ -109,7 +109,7 @@ if use_prod_static: urls += [ path("static/", serve, {"document_root": settings.STATIC_ROOT}), ] -else: +else: # nocoverage def serve_static(request: HttpRequest, path: str) -> FileResponse: response = staticfiles_serve(request, path) diff --git a/zproject/urls.py b/zproject/urls.py index e32deb041c..4c627961fb 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -225,7 +225,7 @@ from zerver.views.zephyr import webathena_kerberos_login from zproject import dev_urls from zproject.legacy_urls import legacy_urls -if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: +if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: # nocoverage from two_factor.gateways.twilio.urls import urlpatterns as tf_twilio_urls from two_factor.urls import urlpatterns as tf_urls @@ -785,7 +785,7 @@ urls += [ ] # Front-end Sentry requests tunnel through the server, if enabled -if settings.SENTRY_FRONTEND_DSN: +if settings.SENTRY_FRONTEND_DSN: # nocoverage urls += [path("error_tracing", sentry_tunnel)] # User documentation site @@ -818,7 +818,7 @@ urls += [ path("policies/", policy_documentation_view), ] -if not settings.CORPORATE_ENABLED: +if not settings.CORPORATE_ENABLED: # nocoverage # This conditional behavior cannot be tested directly, since # urls.py is not readily reloaded in Django tests. See the block # comment inside apps_view for details. @@ -827,7 +827,7 @@ if not settings.CORPORATE_ENABLED: ] # Two-factor URLs -if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: +if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: # nocoverage urls += [path("", include(tf_urls)), path("", include(tf_twilio_urls))] if settings.DEVELOPMENT: