Add failing tests for LookupIndex context manager protocol (#143) [ON HOLD] by turbomam · Pull Request #144 · linkml/linkml-map

turbomam · 2026-03-09T17:42:50Z

Status: ON HOLD — waiting for #143 implementation

These tests define the expected contract for #143. They will fail CI until the implementation lands. This PR should be merged after #143 is implemented, or the implementation can be done directly in this branch.

Summary

Adds 3 tests that currently fail — they describe the expected behavior for #143 (LookupIndex context manager + resource cleanup in transform_spec).

What fails and why

Test	Failure	Root cause
`test_context_manager_basic`	`AttributeError: __enter__`	`LookupIndex` has no `__enter__`/`__exit__`
`test_context_manager_cleans_up_on_exception`	`AttributeError: __enter__`	Same
`test_transform_spec_closes_lookup_index`	`DID NOT RAISE`	`transform_spec` never calls `close()` — the DuckDB connection is still usable after iteration

Proposed fix (for #143)

Two small changes:

Add __enter__/__exit__ to LookupIndex
Add cleanup in transform_spec after the outer loop

Happy to implement this if you'd like, or feel free to take it — the fix is mechanical.

LookupIndex: add context manager protocol and resource cleanup in transform_spec #143 — the issue describing the resource leak
Add cross-table lookup support for join-based transformations #136 — the cross-table lookup PR where these components were introduced (now merged)
Add edge-case tests for cross-table lookup (#136) #142 — companion PR with 11 passing edge-case tests (merged)

Copilot

Pull request overview

Adds new “test-first” coverage defining the expected contract for Issue #143 (LookupIndex context manager protocol and transform_spec resource cleanup), without changing production code.

Changes:

Introduces tests asserting LookupIndex supports with via __enter__/__exit__ and cleans up on exceptions.
Adds an integration-style test asserting transform_spec closes the LookupIndex after iteration completes.
Embeds minimal inline source/target schemas + TSV fixtures needed to exercise the cleanup path.

Copilot · 2026-03-09T17:47:27Z

tests/test_utils/test_lookup_index_context_manager.py

+"""Tests for LookupIndex context manager protocol and resource cleanup.
+
+These tests verify that LookupIndex supports ``with`` statement usage
+and that transform_spec properly cleans up its LookupIndex. Currently,
+LookupIndex does NOT implement __enter__/__exit__, so these tests are
+expected to fail until the protocol is added.
+
+See: https://github.com/linkml/linkml-map/issues/143


These tests are intentionally failing (per docstring), which will break the main test suite if this PR is merged on its own. Consider marking them with pytest.mark.xfail (with a link/reason for #143) or skipping until the implementation PR lands, or alternatively include the implementation changes in the same PR so CI stays green.

Copilot · 2026-03-09T17:47:28Z

tests/test_utils/test_lookup_index_context_manager.py

+    # After exiting the context, the connection should be closed
+    assert not idx.is_registered("data")
+


The docstring says the DuckDB connection should be closed after exiting the context, but the assertion only checks that the table is no longer registered (which could be satisfied without closing the connection). To make this test actually enforce resource cleanup, add an assertion that a DuckDB operation (e.g., register_table/lookup_row) fails after the with block, ideally by asserting on duckdb.Error (or similar).

Copilot · 2026-03-09T17:47:28Z

tests/test_utils/test_lookup_index_context_manager.py

+    # Connection should still be cleaned up
+    assert not idx.is_registered("data")
+


Same as above: this only checks that _tables was cleared, not that the DuckDB connection was actually closed. To validate cleanup on exceptions, consider also asserting that a subsequent DuckDB operation on idx fails after the with exits (e.g., register_table/lookup_row raising duckdb.Error).

Copilot · 2026-03-09T17:47:28Z

tests/test_utils/test_lookup_index_context_manager.py

+    # After transform_spec completes, the LookupIndex should be closed.
+    # Attempting to use it should fail.
+    with pytest.raises((duckdb.ConnectionException, duckdb.InvalidInputException)):
+        tr.lookup_index.register_table(
+            "should_fail", tmp_path / "sites.tsv", "site_code"
+        )


This assertion is a bit brittle because it assumes (1) transform_spec leaves a non-None lookup_index instance attached to the transformer, and (2) DuckDB raises one of these two specific exception types when the connection is closed. A cleanup implementation that sets transformer.lookup_index = None, or a DuckDB version that raises a different subclass, would cause this test to fail despite correct cleanup. Consider accepting AttributeError/None and/or asserting on the broader duckdb.Error base class instead of specific subclasses.

Address review suggestions from Copilot on PR #144: - Context manager tests now also assert that DuckDB operations raise after the `with` block exits, not just that is_registered() returns False. This verifies the connection is actually closed, not just that the internal table dict was cleared. - transform_spec cleanup test now accepts EITHER implementation strategy: setting lookup_index to None OR calling close(). This avoids brittleness if the fix nulls the attribute instead of closing the connection. All 3 tests still fail as expected on the cross-table-lookup branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

turbomam · 2026-03-09T17:59:08Z

Addressed Copilot's inline feedback in 716ef0b:

Context manager tests — now also assert that register_table() raises ConnectionException/InvalidInputException after the with block exits, verifying the DuckDB connection is actually closed (not just the internal _tables dict cleared).
transform_spec cleanup test — now accepts either cleanup strategy: lookup_index = None (nulling) OR close() (connection raises). Either is a valid implementation of LookupIndex: add context manager protocol and resource cleanup in transform_spec #143.

All 3 tests still fail as expected.

Address review suggestions from Copilot on PR #144: - Context manager tests now also assert that DuckDB operations raise after the `with` block exits, not just that is_registered() returns False. This verifies the connection is actually closed, not just that the internal table dict was cleared. - transform_spec cleanup test now accepts EITHER implementation strategy: setting lookup_index to None OR calling close(). This avoids brittleness if the fix nulls the attribute instead of closing the connection. All 3 tests still fail as expected on the cross-table-lookup branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

These 3 tests document the expected behavior described in #143: - LookupIndex should support `with` statement (context manager protocol) - LookupIndex should clean up on exception within context - transform_spec should close LookupIndex after iteration completes All 3 tests currently FAIL because LookupIndex lacks __enter__/__exit__ and transform_spec does not call close(). They will pass once #143 is implemented. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Address review suggestions from Copilot on PR #144: - Context manager tests now also assert that DuckDB operations raise after the `with` block exits, not just that is_registered() returns False. This verifies the connection is actually closed, not just that the internal table dict was cleared. - transform_spec cleanup test now accepts EITHER implementation strategy: setting lookup_index to None OR calling close(). This avoids brittleness if the fix nulls the attribute instead of closing the connection. All 3 tests still fail as expected on the cross-table-lookup branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…spec LookupIndex now supports `with` statement usage via __enter__/__exit__. transform_spec() closes the LookupIndex it creates after iteration completes, preventing DuckDB connection leaks. Closes #143 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

amc-corey-cox · 2026-03-09T21:43:25Z

@turbomam I think I was able to fix this on main and now we should be able to merge this. Does this cover what you intended to cover still?

amc-corey-cox

This looks good to me.

Copilot AI review requested due to automatic review settings March 9, 2026 17:42

turbomam mentioned this pull request Mar 9, 2026

LookupIndex: add context manager protocol and resource cleanup in transform_spec #143

Open

Copilot started reviewing on behalf of turbomam March 9, 2026 17:43 View session

turbomam mentioned this pull request Mar 9, 2026

Add cross-table lookup support for join-based transformations #136

Merged

4 tasks

Copilot AI reviewed Mar 9, 2026

View reviewed changes

amc-corey-cox force-pushed the lookup-index-context-manager branch from 0b74ba2 to 760a141 Compare March 9, 2026 18:41

amc-corey-cox changed the base branch from cross-table-lookup to main March 9, 2026 18:41

amc-corey-cox closed this Mar 9, 2026

amc-corey-cox reopened this Mar 9, 2026

turbomam force-pushed the lookup-index-context-manager branch 2 times, most recently from 8e7ec33 to 6100bc1 Compare March 9, 2026 20:24

turbomam changed the title ~~Add failing tests for LookupIndex context manager protocol (#143)~~ Add failing tests for LookupIndex context manager protocol (#143) [ON HOLD] Mar 9, 2026

turbomam marked this pull request as draft March 9, 2026 20:31

amc-corey-cox force-pushed the lookup-index-context-manager branch from f33a79e to c718189 Compare March 9, 2026 21:38

turbomam and others added 3 commits March 9, 2026 16:40

amc-corey-cox force-pushed the lookup-index-context-manager branch from c718189 to 75a6a93 Compare March 9, 2026 21:41

amc-corey-cox approved these changes Mar 9, 2026

View reviewed changes

amc-corey-cox mentioned this pull request Mar 17, 2026

Support cross-table slot lookup in class_derivation slot_derivations #134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add failing tests for LookupIndex context manager protocol (#143) [ON HOLD]#144

Add failing tests for LookupIndex context manager protocol (#143) [ON HOLD]#144
turbomam wants to merge 3 commits intomainfrom
lookup-index-context-manager

turbomam commented Mar 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

turbomam commented Mar 9, 2026

Uh oh!

amc-corey-cox commented Mar 9, 2026

Uh oh!

amc-corey-cox left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		# After exiting the context, the connection should be closed
		assert not idx.is_registered("data")

		# Connection should still be cleaned up
		assert not idx.is_registered("data")

Conversation

turbomam commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status: ON HOLD — waiting for #143 implementation

Summary

What fails and why

Proposed fix (for #143)

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

turbomam commented Mar 9, 2026

Uh oh!

amc-corey-cox commented Mar 9, 2026

Uh oh!

amc-corey-cox left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

turbomam commented Mar 9, 2026 •

edited

Loading