Skip to content

test: reduce and improve tests in src/compile/types.rs#1173

Merged
jamesadevine merged 2 commits into
mainfrom
test-reducer/types-cycle5-8d1bd6f614e4924b
Jun 24, 2026
Merged

test: reduce and improve tests in src/compile/types.rs#1173
jamesadevine merged 2 commits into
mainfrom
test-reducer/types-cycle5-8d1bd6f614e4924b

Conversation

@github-actions

@github-actions github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Improves the test suite in src/compile/types.rs by strengthening weak tests and restoring genuine coverage that had been incorrectly identified as redundant.

Strengthened tests

Test Action Reason
test_supply_chain_feed_only_validates Strengthened Now also asserts that a combined feed + registry-with-connection config validates OK, exercising more of the validation logic
test_permissions_default Rewritten Now deserialises permissions: {} from YAML instead of calling Default::default() — tests the actual serde contract and guards against accidentally adding required fields

Retained tests (with corrections)

Test Action Reason
test_frontmatter_inlined_imports_false_explicit Kept Only test of the explicit inlined-imports: false override path — distinct from the default (absent key) path
test_cache_memory_bool_false Kept with full assertions assert!(cm.allowed_extensions().is_empty()) retained — verifies the Enabled variant's accessor returns no extensions, a distinct path from is_enabled()
test_lean_bool_false Kept with full assertions assert!(lean.toolchain().is_none()) retained — verifies the Enabled variant returns None; not covered by test_lean_with_toolchain which only exercises WithOptions

Test plan

  • cargo test: all tests pass ✅
  • cargo clippy --all-targets --all-features: no warnings or errors ✅

- Remove test_frontmatter_inlined_imports_false_explicit: duplicate of
  test_frontmatter_inlined_imports_defaults_to_false — both assert
  key and explicit `false` are indistinguishable.

- Strengthen test_supply_chain_feed_only_validates: the original assertion
  `validate().is_ok()` on a feed-only config was vacuous because
  `validate()` only errors when `registry` is Some without a connection.
  Now also tests combined feed + registry-with-connection, which exercises
  more of the validation logic.

- Rewrite test_permissions_default: previously called
  `PermissionsConfig::default()` which is only ever called from this test
  (never in production). Replaced with deserialization of `permissions: {}`,
  which guards against accidentally adding a required field or a non-None
  serde default — a meaningful regression check.

- Remove vacuous `allowed_extensions().is_empty()` assertion from
  test_cache_memory_bool_false: `CacheMemoryToolConfig::Enabled(_)` always
  returns `&[]` regardless of the bool, so this assertion can never fail.
  The coverage is already provided by test_cache_memory_with_options.

- Remove vacuous `toolchain().is_none()` assertion from test_lean_bool_false:
  `LeanRuntimeConfig::Enabled(_)` always returns `None` from `toolchain()`;
  only the `WithOptions` variant can return `Some`. That case is covered by
  test_lean_with_toolchain.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot mentioned this pull request Jun 24, 2026
@jamesadevine

Copy link
Copy Markdown
Collaborator

@copilot The new assertions are welcome:

• the combined  feed  +  registry -with- service-connection   validate()  check in  test_supply_chain_feed_only_validates 
• switching  test_permissions_default  to deserialize  permissions: {}  (nicely guards against a future required field or a non- None  serde default)

However, three deletions drop real, non-redundant coverage:

  1.  test_frontmatter_inlined_imports_false_explicit  (removed entirely) — this is the only test of the explicit  inlined-imports: false  override path. The remaining test covers the default/true case, so the false override is now untested. Please keep it.
  2.  assert!(cm.allowed_extensions().is_empty())  (removed) — verifies that a disabled  cache-memory  config reports no allowed extensions. That's a distinct accessor path from  is_enabled() ; keeping the assertion costs nothing.
  3.  assert!(lean.toolchain().is_none())  (removed) — this asserted the  Enabled  variant returns  None . The replacement comment says it's "covered by  test_lean_with_toolchain ", but that test exercises the  WithOptions  variant, not the  Enabled → None  path — so this is a genuine coverage gap, not a duplicate.

Could you restore these three? Trimming the truly redundant bits is fine — e.g. dropping  test_generate_stage_prefix -style single-word duplicates — but #1098 was just closed for removing the only coverage of a specific behavior, and items 1 and 3 here are the same pattern. Happy to merge once they're back in alongside the new assertions.

Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>

Copilot AI commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@copilot The new assertions are welcome:

• the combined  feed  +  registry -with- service-connection   validate()  check in  test_supply_chain_feed_only_validates 
...

Restored all three in commit test: restore three dropped coverage items per review feedback:

  1. test_frontmatter_inlined_imports_false_explicit — test is back, covering the explicit inlined-imports: false override path.
  2. assert!(cm.allowed_extensions().is_empty()) — restored in test_cache_memory_bool_false.
  3. assert!(lean.toolchain().is_none()) — restored in test_lean_bool_false (and the comment replacing it removed).

Copilot AI requested a review from jamesadevine June 24, 2026 15:34
@jamesadevine jamesadevine marked this pull request as ready for review June 24, 2026 15:38
@jamesadevine jamesadevine merged commit 237cd36 into main Jun 24, 2026
21 checks passed
@jamesadevine jamesadevine deleted the test-reducer/types-cycle5-8d1bd6f614e4924b branch June 24, 2026 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants