From e1b1d27c384a94392096cfb3445ce31985fdb559 Mon Sep 17 00:00:00 2001 From: bhuvan-somisetty Date: Wed, 30 Sep 2026 22:15:03 +0530 Subject: [PATCH] grass.pygrass: Fix Ilist attribute access, indexing, and append issues In `python/grass/pygrass/vector/basic.py`, `Ilist.extend()` and `Ilist.remove()` incorrectly attempted to access `ilist.ilist` and `value.ilist`, which do not exist on `Ilist` instances (the C pointer is stored in `c_ilist`), resulting in an `AttributeError`. Additionally: - `Ilist.append()` previously used a bare `raise` when `Vect_list_append()` failed; raise `GrassError` from `grass.pygrass.errors` and coerce `value` with `int()`. - `Ilist.__getitem__()` negative index calculation allowed negative out-of-bounds lookups without raising `IndexError`. - `Ilist.__contains__()` now delegates directly to `contains()` using the C routine. - Add comprehensive pytest unit tests in `python/grass/pygrass/vector/tests/test_basic.py`. Fixes #7976. Signed-off-by: bhuvan-somisetty --- python/grass/pygrass/vector/basic.py | 18 +- .../grass/pygrass/vector/tests/test_basic.py | 155 ++++++++++++++++++ 2 files changed, 164 insertions(+), 9 deletions(-) create mode 100644 python/grass/pygrass/vector/tests/test_basic.py diff --git a/python/grass/pygrass/vector/basic.py b/python/grass/pygrass/vector/basic.py index b1ef9ae0dfa..088fadf8059 100644 --- a/python/grass/pygrass/vector/basic.py +++ b/python/grass/pygrass/vector/basic.py @@ -5,9 +5,10 @@ """ import ctypes -import grass.lib.vector as libvect from collections.abc import Iterable +import grass.lib.vector as libvect +from grass.pygrass.errors import GrassError from grass.pygrass.shell.conversion import dict2html @@ -306,7 +307,7 @@ def __getitem__(self, key): if isinstance(key, int): if key < 0: # Handle negative indices key += self.c_ilist.contents.n_values - if key >= self.c_ilist.contents.n_values: + if key >= self.c_ilist.contents.n_values or key < 0: msg = "Index out of range" raise IndexError(msg) return self.c_ilist.contents.value[key] @@ -328,12 +329,13 @@ def __repr__(self): return "Ilist(%r)" % list(self.__iter__()) def __contains__(self, item): - return item in self.__iter__() + return self.contains(item) def append(self, value): """Append an integer to the list""" - if libvect.Vect_list_append(self.c_ilist, value): - raise # TODO + if libvect.Vect_list_append(self.c_ilist, int(value)): + msg = "Cannot append value to list" + raise GrassError(msg) def reset(self): """Reset the list""" @@ -347,7 +349,7 @@ def extend(self, ilist): :type ilist: a Ilist object """ if isinstance(ilist, Ilist): - libvect.Vect_list_append_list(self.c_ilist, ilist.ilist) + libvect.Vect_list_append_list(self.c_ilist, ilist.c_ilist) else: for i in ilist: self.append(i) @@ -357,7 +359,7 @@ def remove(self, value): if isinstance(value, int): libvect.Vect_list_delete(self.c_ilist, value) elif isinstance(value, Ilist): - libvect.Vect_list_delete_list(self.c_ilist, value.ilist) + libvect.Vect_list_delete_list(self.c_ilist, value.c_ilist) elif isinstance(value, Iterable): for i in value: libvect.Vect_list_delete(self.c_ilist, int(i)) @@ -546,8 +548,6 @@ def from_string(self, string): """ num_errors = libvect.Vect_str_to_cat_list(string, self.c_cat_list) if num_errors: - from grass.pygrass.errors import GrassError - raise GrassError("%d number of errors in ranges" % num_errors) def from_array(self, array): diff --git a/python/grass/pygrass/vector/tests/test_basic.py b/python/grass/pygrass/vector/tests/test_basic.py new file mode 100644 index 00000000000..514aaef4820 --- /dev/null +++ b/python/grass/pygrass/vector/tests/test_basic.py @@ -0,0 +1,155 @@ +""" +SPDX-FileCopyrightText: 2026 GRASS Development Team +SPDX-License-Identifier: GPL-2.0-or-later + +Unit tests for grass.pygrass.vector.basic.Ilist +""" + +from unittest.mock import patch + +import pytest + +from grass.pygrass.errors import GrassError +from grass.pygrass.vector.basic import Ilist + + +def test_init_empty(): + """Test empty Ilist initialization.""" + ilist = Ilist() + assert len(ilist) == 0 + assert list(ilist) == [] + + +def test_init_with_list(): + """Test Ilist initialization with integer list.""" + ilist = Ilist([1, 2, 3]) + assert len(ilist) == 3 + assert list(ilist) == [1, 2, 3] + + +def test_init_with_ilist(): + """Test Ilist initialization from another Ilist instance.""" + orig = Ilist([1, 2, 3]) + ilist = Ilist(orig) + assert len(ilist) == 3 + assert list(ilist) == [1, 2, 3] + + +def test_getitem(): + """Test __getitem__ with indexing and slicing.""" + ilist = Ilist([10, 20, 30, 40]) + assert ilist[0] == 10 + assert ilist[1] == 20 + assert ilist[-1] == 40 + assert ilist[1:3] == [20, 30] + with pytest.raises(IndexError): + _ = ilist[10] + with pytest.raises(IndexError): + _ = ilist[-10] + + +def test_setitem(): + """Test __setitem__.""" + ilist = Ilist([1, 2, 3]) + ilist[1] = 20 + assert list(ilist) == [1, 20, 3] + with pytest.raises(ValueError, match="already in the list"): + ilist[0] = 3 + + +def test_contains(): + """Test contains and in operator.""" + ilist = Ilist([1, 2, 3]) + assert 1 in ilist + assert 4 not in ilist + assert ilist.contains(2) + assert not ilist.contains(5) + + +def test_append(): + """Test appending an integer.""" + ilist = Ilist([1, 2]) + ilist.append(3) + assert list(ilist) == [1, 2, 3] + # Appending duplicate should not duplicate + ilist.append(2) + assert list(ilist) == [1, 2, 3] + # Appending value convertible to int + ilist.append(4.0) + assert list(ilist) == [1, 2, 3, 4] + + +def test_append_error(): + """Test append raises GrassError when Vect_list_append fails.""" + ilist = Ilist() + # Vect_list_append only fails on memory allocation failure (or NULL list), + # which cannot be easily provoked in a unit test, so mock the return code. + with ( + patch( + "grass.pygrass.vector.basic.libvect.Vect_list_append", + return_value=1, + ), + pytest.raises(GrassError, match="Cannot append value to list"), + ): + ilist.append(10) + + +def test_extend_with_python_list(): + """Test extending Ilist with Python list.""" + ilist = Ilist([1, 2]) + ilist.extend([3, 4]) + assert list(ilist) == [1, 2, 3, 4] + + +def test_extend_with_ilist(): + """Test extending Ilist with another Ilist instance.""" + ilist_a = Ilist([1, 2, 3]) + ilist_b = Ilist([4, 5, 6]) + ilist_a.extend(ilist_b) + assert list(ilist_a) == [1, 2, 3, 4, 5, 6] + + +def test_remove_int(): + """Test removing an integer.""" + ilist = Ilist([1, 2, 3, 4]) + ilist.remove(2) + assert list(ilist) == [1, 3, 4] + + +def test_remove_ilist(): + """Test removing items from another Ilist instance.""" + ilist_a = Ilist([1, 2, 3, 4, 5]) + ilist_b = Ilist([2, 4]) + ilist_a.remove(ilist_b) + assert list(ilist_a) == [1, 3, 5] + + +def test_remove_iterable(): + """Test removing items with an iterable.""" + ilist = Ilist([1, 2, 3, 4, 5]) + ilist.remove([1, 3]) + assert list(ilist) == [2, 4, 5] + + +def test_remove_invalid(): + """Test removing unsupported value type raises ValueError.""" + ilist = Ilist([1, 2]) + # Pass non-iterable types so execution reaches the unsupported type branch + with pytest.raises(ValueError, match="is not supported"): + ilist.remove(1.5) + with pytest.raises(ValueError, match="is not supported"): + ilist.remove(None) + + +def test_reset(): + """Test resetting Ilist.""" + ilist = Ilist([1, 2, 3]) + ilist.reset() + assert len(ilist) == 0 + assert list(ilist) == [] + + +def test_repr(): + """Test __repr__.""" + ilist = Ilist([1, 2, 3]) + assert repr(ilist) == "Ilist([1, 2, 3])"