test quality: rubber-stamping, coverage gaps, and improvement opportunities #29

New Issue

2026-04-29T22:46:44+10:00

unkinben commented

2026-04-29 22:46:44 +10:00

Rubber-stamping (tests with no real coverage value)

test_config.py

test_returns_empty_dict_when_no_cache_key — tests Python's dict.get() default, not application logic
test_excludes_package_defaults — duplicate of test_returns_empty_when_key_absent, same code path

test_cache.py

Three "hash is 16 chars" tests — redundant when deterministic key format tests already pin the exact string
test_different_paths_produce_different_keys — re-exercises the hash function already covered elsewhere

test_docker_auth.py

test_key_contains_all_components, test_different_services_give_different_keys, test_different_scopes_give_different_keys — testing Python string formatting
test_returns_none_for_non_bearer_header — duplicates parse tests through a thin wrapper

test_routes.py

test_root_contains_version — checks key exists, not that the value is correct
test_flush_specific_remote_echoes_remote — verifies input round-trips through a struct, proves nothing about flush logic
test_flush_all_returns_flushed_structure — asserts key shapes during a no-op where nothing is flushed

test_storage.py

test_returns_http_url_for_secure_storage — secure has no effect on get_url, the test documents a bug without catching regressions
test_different_remotes_give_different_keys, test_different_directories_give_different_keys, test_docker_blob_different_digests_different_keys — testing string inequality

test_virtual.py

test_path_error_is_non_empty_string, test_output_is_valid_yaml, test_merge_returns_bytes — trivial/redundant
test_helm_handler_is_registered — asserting a dict literal contains a key

Biggest missing coverage gaps

test_routes.py (worst offender)

local.upload success path, 409 (already exists), and 500 (S3 rollback) paths — all untested
local.delete success, not-found, and S3 failure paths
discovery module — entire POST /api/v1/artifacts/cache and GET /api/v1/artifacts/{remote} routes are dark
flush with actual S3 objects and per-type targeting
Docker blob content-type branch and OCI index manifest inference

test_cache.py

cleanup_expired_index — completely untested despite being the most complex method
Redis exception swallowing — none of the silent except fallbacks are verified

test_storage.py

exists, get_presigned_url, delete_object — zero tests
download_object success path never reached (only 404 path tested)
CA bundle and bucket-creation logic

test_config.py

get_s3_config, get_redis_config, get_database_config — all untested including error branches with specific missing-var messages
JSON config file loading path

Most impactful improvements

test_docker_auth.py test_store_expires_30s_before_stated_time — freeze time instead of using ±2s tolerance; currently non-deterministic
test_cache.py test_store_and_retrieve_etag/last_modified — mocks aren't state-aware so they don't verify the data written, only the shape returned
test_routes.py test_flush_all_deletes_redis_keys — asserts call count but not the flushed key count in the response body
test_virtual.py test_cache_miss_marks_index_cached — checks it was called, not what args were passed
test_config.py test_no_reload_when_file_unchanged — should patch _load_config and assert call_count == 0 to actually prove no reload happened

## Rubber-stamping (tests with no real coverage value) **test_config.py** - `test_returns_empty_dict_when_no_cache_key` — tests Python's `dict.get()` default, not application logic - `test_excludes_package_defaults` — duplicate of `test_returns_empty_when_key_absent`, same code path **test_cache.py** - Three "hash is 16 chars" tests — redundant when deterministic key format tests already pin the exact string - `test_different_paths_produce_different_keys` — re-exercises the hash function already covered elsewhere **test_docker_auth.py** - `test_key_contains_all_components`, `test_different_services_give_different_keys`, `test_different_scopes_give_different_keys` — testing Python string formatting - `test_returns_none_for_non_bearer_header` — duplicates parse tests through a thin wrapper **test_routes.py** - `test_root_contains_version` — checks key exists, not that the value is correct - `test_flush_specific_remote_echoes_remote` — verifies input round-trips through a struct, proves nothing about flush logic - `test_flush_all_returns_flushed_structure` — asserts key shapes during a no-op where nothing is flushed **test_storage.py** - `test_returns_http_url_for_secure_storage` — `secure` has no effect on `get_url`, the test documents a bug without catching regressions - `test_different_remotes_give_different_keys`, `test_different_directories_give_different_keys`, `test_docker_blob_different_digests_different_keys` — testing string inequality **test_virtual.py** - `test_path_error_is_non_empty_string`, `test_output_is_valid_yaml`, `test_merge_returns_bytes` — trivial/redundant - `test_helm_handler_is_registered` — asserting a dict literal contains a key --- ## Biggest missing coverage gaps **test_routes.py** (worst offender) - `local.upload` success path, 409 (already exists), and 500 (S3 rollback) paths — all untested - `local.delete` success, not-found, and S3 failure paths - `discovery` module — entire `POST /api/v1/artifacts/cache` and `GET /api/v1/artifacts/{remote}` routes are dark - `flush` with actual S3 objects and per-type targeting - Docker blob `content-type` branch and OCI index manifest inference **test_cache.py** - `cleanup_expired_index` — completely untested despite being the most complex method - Redis exception swallowing — none of the silent `except` fallbacks are verified **test_storage.py** - `exists`, `get_presigned_url`, `delete_object` — zero tests - `download_object` success path never reached (only 404 path tested) - CA bundle and bucket-creation logic **test_config.py** - `get_s3_config`, `get_redis_config`, `get_database_config` — all untested including error branches with specific missing-var messages - JSON config file loading path --- ## Most impactful improvements 1. **test_docker_auth.py** `test_store_expires_30s_before_stated_time` — freeze time instead of using ±2s tolerance; currently non-deterministic 2. **test_cache.py** `test_store_and_retrieve_etag/last_modified` — mocks aren't state-aware so they don't verify the data written, only the shape returned 3. **test_routes.py** `test_flush_all_deletes_redis_keys` — asserts call count but not the flushed key count in the response body 4. **test_virtual.py** `test_cache_miss_marks_index_cached` — checks it was called, not what args were passed 5. **test_config.py** `test_no_reload_when_file_unchanged` — should patch `_load_config` and assert `call_count == 0` to actually prove no reload happened

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/artifactapi#29