Copilot commented on code in PR #2366:
URL: https://github.com/apache/age/pull/2366#discussion_r3066032116
##########
drivers/python/test_age_py.py:
##########
@@ -28,6 +29,76 @@
TEST_GRAPH_NAME = "test_graph"
+class TestSetUpAge(unittest.TestCase):
+ """Unit tests for setUpAge() skip_load parameter — no DB required."""
+
+ def _make_mock_conn(self):
+ mock_conn = unittest.mock.MagicMock()
+ mock_cursor = unittest.mock.MagicMock()
+ mock_conn.cursor.return_value.__enter__ =
unittest.mock.Mock(return_value=mock_cursor)
+ mock_conn.cursor.return_value.__exit__ =
unittest.mock.Mock(return_value=False)
+ mock_conn.adapters = unittest.mock.MagicMock()
+ mock_type_info = unittest.mock.MagicMock()
+ mock_type_info.oid = 1
+ mock_type_info.array_oid = 2
+ return mock_conn, mock_cursor, mock_type_info
+
+ def test_skip_load_true_does_not_execute_load(self):
+ """When skip_load=True, LOAD 'age' must not be executed."""
+ mock_conn, mock_cursor, mock_type_info = self._make_mock_conn()
+ with unittest.mock.patch("age.age.TypeInfo.fetch",
return_value=mock_type_info), \
+ unittest.mock.patch("age.age.checkGraphCreated"):
+ age.age.setUpAge(mock_conn, "test_graph", skip_load=True)
+ mock_cursor.execute.assert_called_once_with(
+ "SET search_path = ag_catalog, '$user', public;"
+ )
Review Comment:
These new tests assert the driver sets `SET search_path = ag_catalog,
'$user', public;`, but PostgreSQL's special `$user` placeholder requires double
quotes (`"$user"`) to be expanded. As written, the tests lock in the
likely-buggy single-quoted form; if the driver is corrected to `"$user"`,
update these assertions to match.
##########
drivers/python/age/age.py:
##########
@@ -137,12 +137,20 @@ def load(self, data: bytes | bytearray | memoryview) ->
Any | None:
return parseAgeValue(data_bytes.decode('utf-8'))
-def setUpAge(conn:psycopg.connection, graphName:str,
load_from_plugins:bool=False):
+def setUpAge(conn:psycopg.connection, graphName:str,
load_from_plugins:bool=False, skip_load:bool=False):
+ if skip_load and load_from_plugins:
+ raise ValueError(
+ "skip_load=True and load_from_plugins=True are contradictory. "
+ "Set skip_load=False to load the extension from the plugins path, "
+ "or remove load_from_plugins to skip loading entirely."
+ )
+
with conn.cursor() as cursor:
- if load_from_plugins:
- cursor.execute("LOAD '$libdir/plugins/age';")
- else:
- cursor.execute("LOAD 'age';")
+ if not skip_load:
+ if load_from_plugins:
+ cursor.execute("LOAD '$libdir/plugins/age';")
+ else:
+ cursor.execute("LOAD 'age';")
cursor.execute("SET search_path = ag_catalog, '$user', public;")
Review Comment:
`SET search_path` uses `'$user'` (single quotes), which PostgreSQL treats as
a literal schema name rather than the special `$user` placeholder. This likely
prevents the current user's schema from being included in `search_path` and
diverges from the documented setup elsewhere in the repo (which uses
`"$user"`). Consider changing this statement to use `"$user"` so it behaves as
intended, and update tests accordingly.
```suggestion
cursor.execute('SET search_path = ag_catalog, "$user", public;')
```
##########
drivers/python/test_age_py.py:
##########
@@ -28,6 +29,76 @@
TEST_GRAPH_NAME = "test_graph"
+class TestSetUpAge(unittest.TestCase):
+ """Unit tests for setUpAge() skip_load parameter — no DB required."""
+
+ def _make_mock_conn(self):
+ mock_conn = unittest.mock.MagicMock()
+ mock_cursor = unittest.mock.MagicMock()
+ mock_conn.cursor.return_value.__enter__ =
unittest.mock.Mock(return_value=mock_cursor)
+ mock_conn.cursor.return_value.__exit__ =
unittest.mock.Mock(return_value=False)
+ mock_conn.adapters = unittest.mock.MagicMock()
Review Comment:
The CI workflow runs `python test_age_py.py ...`, which executes only the
custom `TestSuite` built under `if __name__ == '__main__'` and does not include
`TestSetUpAge`. As a result, these unit tests for `skip_load` won’t run in CI.
Consider adding `TestSetUpAge` to the scripted suite, or switching the script
to `unittest.main()` (and passing through args) so discovery runs all tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
