[From PR #175] Add test coverage for remove, mapv, empty?, and count on sets

[andreasronge]

Context

From PR #175 review: The implementation correctly added MapSet-specific clauses for remove/2, mapv/2, empty?/1, and count/1 in lib/ptc_runner/lisp/runtime.ex, but these operations lack dedicated tests.

Current State

Implementation (all correct):

• remove/2 at runtime.ex:33 - def remove(pred, %MapSet{} = set), do: Enum.reject(set, pred)
• mapv/2 at runtime.ex:48 - def mapv(f, %MapSet{} = set), do: Enum.map(set, f)
• count/1 at runtime.ex:83 - def count(%MapSet{} = set), do: MapSet.size(set)
• empty?/1 at runtime.ex:89 - def empty?(%MapSet{} = set), do: MapSet.size(set) == 0

Existing test coverage: The "collection operations on sets" describe block at test/ptc_runner/lisp/eval_test.exs:1023 tests map, filter, and contains? on sets. The four operations listed above have no dedicated tests.

Implementation

Add tests to test/ptc_runner/lisp/eval_test.exs in the existing "collection operations on sets" describe block (around line 1023):

test "remove on set returns vector" do
  {:ok, result, _} = run(~S"(remove odd? #{1 2 3 4})")
  assert is_list(result)
  assert Enum.sort(result) == [2, 4]
end

test "mapv on set returns vector" do
  {:ok, result, _} = run(~S"(mapv inc #{1 2 3})")
  assert is_list(result)
  assert Enum.sort(result) == [2, 3, 4]
end

test "count returns set size" do
  {:ok, result, _} = run(~S"(count #{})")
  assert result == 0

  {:ok, result, _} = run(~S"(count #{1 2 3})")
  assert result == 3
end

test "empty? on sets" do
  {:ok, result, _} = run(~S"(empty? #{})")
  assert result == true

  {:ok, result, _} = run(~S"(empty? #{1})")
  assert result == false
end

Note: Use ~S sigil for set literal syntax to avoid escaping issues.

Acceptance Criteria

• All 4 tests added to "collection operations on sets" describe block
• Tests use strong assertions (specific values, not just shape matching)
• Tests pass: mix test test/ptc_runner/lisp/eval_test.exs
• Code formatted: mix format

Benefits

• Verifies MapSet-specific code paths work correctly
• Catches potential regressions if MapSet clause ordering changes
• Documents expected behavior for these operations on sets
• Completes test coverage for all MapSet clauses added in PR fix(#174): Add MapSet-safe collection operations and set runtime support #175

Notes

• This is a nice-to-have improvement, not a blocking issue
• The implementation code is correct and follows obvious patterns
• The MapSet-specific clauses provide better efficiency than falling through to the generic Enum-based implementations

[andreasronge]

Issue Review: Add test coverage for remove, mapv, empty?, and count on sets

Summary: This issue requests adding 5 tests for MapSet-specific clauses in the Lisp runtime that were added in PR #175 but lack dedicated test coverage.

Pre-Review Checks:

• Codebase health: pass (1010 tests passing)
• Higher-priority blockers: none (no open bugs; issue [From PR #172] Split eval_test.exs into multiple files #173 is tech-debt but not blocking)
• Issue claims verified: yes - all 4 MapSet clauses exist at specified line numbers in lib/ptc_runner/lisp/runtime.ex

Analysis:

1. Should This Be Done?: Yes. Test coverage for new code paths is valuable. The MapSet clauses exist specifically to handle sets efficiently, and having tests ensures they work and catches regressions.

2. Simplifications: None needed - this is straightforward test addition.

3. Codebase Research: Verified:

   • MapSet clauses exist at lines 33, 48, 83, 89 in runtime.ex
   • Existing "collection operations on sets" describe block at eval_test.exs:1023 only tests map, filter, and contains?
   • No existing tests for remove, mapv, count, or empty? on sets

4. Reuse Opportunities: Tests follow the existing pattern in the describe block (using run/1 helper and sorting results due to set ordering).

5. Refactoring Needs: None. Note: Issue [From PR #172] Split eval_test.exs into multiple files #173 suggests splitting eval_test.exs, but that's independent of this work.

6. Edge Cases: Added empty set test cases for count and empty? which weren't in the original suggestion.

7. Issue Scope: Perfect size - 4-5 simple tests in one location.

8. Test Plan: Tests follow testing guidelines (strong assertions with specific values, not just shape matching).

9. Solution Outline: Clear and actionable. A junior developer could implement this.

Changes Made to Issue Body:

• Added ~S sigil note for set literal syntax
• Combined count tests (empty and non-empty sets in one test)
• Combined empty? tests (both cases in one test)
• Added explicit acceptance criteria checklist
• Added exact line number reference for the describe block
• Clarified the benefit of MapSet-specific clauses (efficiency)

Recommended Labels: ready-for-implementation, claude-approved

Verdict: Ready for implementation

[andreasronge]

@claude Please implement this issue.

Implementation Notes:

• Add the 4 tests to the existing "collection operations on sets" describe block in test/ptc_runner/lisp/eval_test.exs (around line 1023)
• Use the ~S sigil for set literal syntax to avoid escaping issues
• Tests should use strong assertions with specific values
• Run mix test test/ptc_runner/lisp/eval_test.exs and mix format before completing

[andreasronge]

Implementation complete! PR #181 adds the 4 requested tests:

✅ remove on set - verifies Enum.reject filters elements correctly
✅ mapv on set - verifies function mapping returns vector with sorted results
✅ empty? on set - verifies both empty and non-empty set cases
✅ count on set - verifies both zero and non-zero counts

All tests follow the existing pattern in the describe block, use strong assertions with specific values, and employ the ~S sigil for set literals. The implementation:

• Adds 28 lines to test/ptc_runner/lisp/eval_test.exs (lines 1041-1067)
• Passes all 1014 tests in precommit
• Code formatted with mix format