Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving Test coverage #365

Merged
merged 18 commits into from
Oct 19, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion tests/containers_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
"""Test the kml classes."""


from fastkml import kml
from fastkml import kml, containers, features
from tests.base import Lxml
from tests.base import StdLibrary
import pytest


class TestStdLibrary(StdLibrary):
Expand Down Expand Up @@ -64,6 +65,36 @@ def test_document_boolean_visibility_invalid(self) -> None:

assert d.features[0].visibility is None
assert d.features[0].isopen

def test_container_creation(self)->None:
container = containers._Container(
ns="ns",
id="id",
target_id="target_id",
name="name"
)
assert container.ns == "ns"
assert container.name == "name"
def test_container_feature_append(self)->None:
container = containers._Container(
ns="ns",
id="id",
target_id="target_id",
name="name"
)
feature = features._Feature(name="new_feature")
assert container.append(feature) is None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this line is needed, as .append always returns None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

with pytest.raises(ValueError):
container.append(container)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Good test for appending features and handling invalid appends

This test covers both successful append and the error case. Consider adding an assertion to check if the feature was actually appended to the container.

Suggested change
def test_container_feature_append(self)->None:
container = containers._Container(
ns="ns",
id="id",
target_id="target_id",
name="name"
)
feature = features._Feature(name="new_feature")
assert container.append(feature) is None
with pytest.raises(ValueError):
container.append(container)
def test_container_feature_append(self) -> None:
container = containers._Container(
ns="ns", id="id", target_id="target_id", name="name"
)
feature = features._Feature(name="new_feature")
container.append(feature)
assert feature in container.features
with pytest.raises(ValueError):
container.append(container)


def test_document_container_get_style_url(self)->None:
document = containers.Document(
name="Document",
ns="ns",
style_url="www.styleurl.com"
)
assert document.get_style_by_url(style_url="www.styleurl.com") is None



class TestLxml(Lxml, TestStdLibrary):
Expand Down
23 changes: 23 additions & 0 deletions tests/geometries/boundaries_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

"""Test the Outer and Inner Boundary classes."""

import pytest
from fastkml.exceptions import GeometryError
import pygeoif.geometry as geo

from fastkml.geometry import Coordinates
Expand All @@ -41,6 +43,16 @@ def test_outer_boundary(self) -> None:
"1.000000,2.000000 2.000000,0.000000 0.000000,0.000000 1.000000,2.000000"
"</kml:coordinates></kml:LinearRing></kml:outerBoundaryIs>"
)
def test_outer_boundry_geometry_error(self) -> None:
"""Test GeometryError."""
p = geo.Point(1, 2)
coords = ((1, 2), (2, 0), (0, 0), (1, 2))

with pytest.raises(GeometryError):
OuterBoundaryIs(
kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
geometry=p
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Good test for GeometryError in OuterBoundaryIs

This test effectively checks for the GeometryError. Consider adding a similar test for other geometry types to ensure consistent error handling.

Suggested change
def test_outer_boundry_geometry_error(self) -> None:
"""Test GeometryError."""
p = geo.Point(1, 2)
coords = ((1, 2), (2, 0), (0, 0), (1, 2))
with pytest.raises(GeometryError):
OuterBoundaryIs(
kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
geometry=p
)
def test_outer_boundary_geometry_error(self) -> None:
"""Test GeometryError for different geometry types."""
geometries = [geo.Point(1, 2), geo.LineString([(0, 0), (1, 1)]), geo.Polygon([(0, 0), (1, 1), (1, 0)])]
coords = ((1, 2), (2, 0), (0, 0), (1, 2))
kml_geometry = LinearRing(kml_coordinates=Coordinates(coords=coords))
for geometry in geometries:
with pytest.raises(GeometryError):
OuterBoundaryIs(kml_geometry=kml_geometry, geometry=geometry)

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix typo in method name and improve docstring.

The new test method is a good addition, but there are a couple of improvements to be made:

  1. Fix the typo in the method name: "boundry" should be "boundary".
  2. The docstring could be more descriptive to explain the purpose of the test.

Here's a suggested improvement:

-    def test_outer_boundry_geometry_error(self) -> None:
-        """Test GeometryError."""
+    def test_outer_boundary_geometry_error(self) -> None:
+        """Test that OuterBoundaryIs raises GeometryError with invalid geometry."""

The rest of the test implementation looks good and correctly checks for the GeometryError exception.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_outer_boundry_geometry_error(self) -> None:
"""Test GeometryError."""
p = geo.Point(1, 2)
coords = ((1, 2), (2, 0), (0, 0), (1, 2))
with pytest.raises(GeometryError):
OuterBoundaryIs(
kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
geometry=p
)
def test_outer_boundary_geometry_error(self) -> None:
"""Test that OuterBoundaryIs raises GeometryError with invalid geometry."""
p = geo.Point(1, 2)
coords = ((1, 2), (2, 0), (0, 0), (1, 2))
with pytest.raises(GeometryError):
OuterBoundaryIs(
kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
geometry=p
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apurvabanka, thank you for making the changes!

(^.^)/


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


def test_read_outer_boundary(self) -> None:
"""Test the from_string method."""
Expand Down Expand Up @@ -72,6 +84,17 @@ def test_inner_boundary(self) -> None:
"1.000000,2.000000 2.000000,0.000000 0.000000,0.000000 1.000000,2.000000"
"</kml:coordinates></kml:LinearRing></kml:innerBoundaryIs>"
)

def test_inner_boundry_geometry_error(self) -> None:
"""Test GeometryError."""
p = geo.Point(1, 2)
coords = ((1, 2), (2, 0), (0, 0), (1, 2))

with pytest.raises(GeometryError):
InnerBoundaryIs(
kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
geometry=p
)
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Fix typo in method name, improve docstring, and consider refactoring.

This test method is a good addition, but there are some improvements to be made:

  1. Fix the typo in the method name: "boundry" should be "boundary".
  2. The docstring could be more descriptive to explain the purpose of the test.
  3. Consider refactoring to reduce code duplication with test_outer_boundary_geometry_error.

Here's a suggested improvement for the method signature and docstring:

-    def test_inner_boundry_geometry_error(self) -> None:
-        """Test GeometryError."""
+    def test_inner_boundary_geometry_error(self) -> None:
+        """Test that InnerBoundaryIs raises GeometryError with invalid geometry."""

To reduce code duplication, consider creating a helper method that both test_outer_boundary_geometry_error and test_inner_boundary_geometry_error can use. For example:

def _test_boundary_geometry_error(self, boundary_class):
    p = geo.Point(1, 2)
    coords = ((1, 2), (2, 0), (0, 0), (1, 2))

    with pytest.raises(GeometryError):
        boundary_class(
            kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
            geometry=p
        )

def test_outer_boundary_geometry_error(self):
    """Test that OuterBoundaryIs raises GeometryError with invalid geometry."""
    self._test_boundary_geometry_error(OuterBoundaryIs)

def test_inner_boundary_geometry_error(self):
    """Test that InnerBoundaryIs raises GeometryError with invalid geometry."""
    self._test_boundary_geometry_error(InnerBoundaryIs)

This refactoring would make the tests more maintainable and reduce redundancy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apurvabanka, thank you for implementing the changes!

(^.^/)


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


def test_read_inner_boundary_multiple_linestrings(self) -> None:
"""
Expand Down
60 changes: 60 additions & 0 deletions tests/geometries/functions_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
from typing_extensions import Protocol
from unittest.mock import Mock, patch
import pytest
from fastkml.exceptions import KMLParseError, KMLWriteError
from fastkml.geometry import handle_invalid_geometry_error, coordinates_subelement, create_kml_geometry
from fastkml.types import Element
from tests.base import StdLibrary
from fastkml import base

class TestGeometryFunctions(StdLibrary):
"""Test functions in Geometry"""

@patch('fastkml.config.etree.tostring')
def test_handle_invalid_geometry_error_true(self, mock_to_string) -> None:
mock_element = Mock()
with pytest.raises(KMLParseError):
handle_invalid_geometry_error(error=ValueError, element=mock_element, strict=True)

@patch('fastkml.config.etree.tostring')
def test_handle_invalid_geometry_error_false(self, mock_to_string) -> None:
mock_element = Mock()
assert handle_invalid_geometry_error(error=ValueError, element=mock_element, strict=False) is None

def test_coordinates_subelement_exception(self) -> None:
obj = Mock()
setattr(obj, 'coordinates', [(1.123456, 2.654321, 3.111111, 4.222222)]) # Invalid 4D coordinates

element = Mock()

precision = None
attr_name = 'coordinates'

with pytest.raises(KMLWriteError):
coordinates_subelement(
obj=obj,
attr_name=attr_name,
node_name=None,
element=element,
precision=precision,
verbosity=None,
default=None
)
def test_coordinates_subelement_getattr(self) -> None:
obj = Mock()
setattr(obj, 'coordinates', [(1.123456, 2.654321), (3.123456, 4.654321)])

element = Mock()

precision = 4
attr_name = 'coordinates'

assert coordinates_subelement(
obj=None,
attr_name=attr_name,
node_name=None,
element=element,
precision=precision,
verbosity=None,
default=None
) is None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Good addition of tests for geometry functions

These tests cover important geometry functions. Consider adding more test cases for different types of geometries and edge cases to ensure comprehensive coverage.

class TestGeometryFunctions(StdLibrary):
    """Test functions in Geometry"""

    @pytest.mark.parametrize("strict, expected", [(True, pytest.raises(KMLParseError)), (False, None)])
    @patch('fastkml.config.etree.tostring')
    def test_handle_invalid_geometry_error(self, mock_to_string, strict, expected):
        mock_element = Mock()
        with expected:
            result = handle_invalid_geometry_error(error=ValueError, element=mock_element, strict=strict)
            if not strict:
                assert result is None

    @pytest.mark.parametrize("coordinates, precision, expected", [
        ([(1.123456, 2.654321, 3.111111, 4.222222)], None, pytest.raises(KMLWriteError)),
        ([(1.123456, 2.654321), (3.123456, 4.654321)], 4, None)
    ])
    def test_coordinates_subelement(self, coordinates, precision, expected):
        obj = Mock(coordinates=coordinates)
        element = Mock()
        with expected:
            result = coordinates_subelement(obj, 'coordinates', None, element, precision, None, None)
            if expected is None:
                assert result is None

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix discrepancy between setup and function call

The test case is well-structured, but there's a discrepancy between the setup and the function call that needs to be addressed.

The obj is set up with valid coordinates, but it's not used in the coordinates_subelement function call. Instead, None is passed as the obj argument. This doesn't test the intended behavior. To fix this, replace:

assert coordinates_subelement(
    obj=None,
    # ... other arguments ...
) is None

with:

assert coordinates_subelement(
    obj=obj,
    # ... other arguments ...
) is None

This change ensures that the test actually uses the mock object with the valid coordinates you've set up.

12 changes: 10 additions & 2 deletions tests/geometries/linestring_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@

from fastkml import exceptions
from fastkml.enums import Verbosity
from fastkml.exceptions import KMLParseError
from fastkml.geometry import LineString
from fastkml.exceptions import GeometryError, KMLParseError
from fastkml.geometry import Coordinates, LineString
from tests.base import Lxml
from tests.base import StdLibrary

Expand All @@ -37,6 +37,14 @@ def test_init(self) -> None:
assert line_string.geometry == ls
assert line_string.altitude_mode is None
assert line_string.extrude is None

def test_geometry_error(self) -> None:
"""Test GeometryError."""
p = geo.Point(1, 2)
q = Coordinates(ns="ns")

with pytest.raises(GeometryError):
LineString(geometry=p, kml_coordinates=q)

def test_to_string(self) -> None:
"""Test the to_string method."""
Expand Down
12 changes: 11 additions & 1 deletion tests/geometries/multigeometry_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

"""Test the geometry classes."""
import pytest
from fastkml.exceptions import GeometryError
import pygeoif.geometry as geo

from fastkml.enums import Verbosity
from fastkml.geometry import MultiGeometry
from fastkml.geometry import Coordinates, MultiGeometry
from tests.base import Lxml
from tests.base import StdLibrary

Expand Down Expand Up @@ -288,6 +290,14 @@ def test_multi_geometries_verbose(self) -> None:
assert xml.count("tessellate>0<") == 12 # points do not have tessellate
assert xml.count("extrude>0<") == 13
assert xml.count("altitudeMode>clampToGround<") == 13

def test_geometry_error(self) -> None:
"""Test GeometryError."""
p = geo.Point(1, 2)
q = Coordinates(ns="ns")

with pytest.raises(GeometryError):
MultiGeometry(geometry=p, kml_geometries=q)

def test_multi_geometries_read(self) -> None:
xml = (
Expand Down
12 changes: 11 additions & 1 deletion tests/geometries/point_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@
import pytest

from fastkml.enums import Verbosity
from fastkml.exceptions import KMLParseError
from fastkml.exceptions import GeometryError, KMLParseError
from fastkml.geometry import Point
from fastkml.geometry import Coordinates
from tests.base import Lxml
from tests.base import StdLibrary

Expand All @@ -38,6 +39,15 @@ def test_init(self) -> None:
assert point.geometry == p
assert point.altitude_mode is None
assert point.extrude is None

def test_geometry_error(self) -> None:
"""Test GeometryError."""
p = geo.Point(1, 2)
q = Coordinates(ns="ns")

with pytest.raises(GeometryError):
Point(geometry=p, kml_coordinates=q)


def test_to_string_2d(self) -> None:
"""Test the to_string method."""
Expand Down
10 changes: 10 additions & 0 deletions tests/geometries/polygon_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

"""Test the geometry classes."""

import pytest
from fastkml.exceptions import GeometryError
import pygeoif.geometry as geo

from fastkml.enums import AltitudeMode
Expand Down Expand Up @@ -126,6 +128,14 @@ def test_to_string_verbose_none(self) -> None:
polygon = Polygon(ns="", geometry=poly)

assert "extrude>0</" in polygon.to_string(verbosity=Verbosity.verbose)

def test_geometry_error(self) -> None:
"""Test GeometryError."""
poly = geo.Polygon([(0, 0), (0, 1), (1, 1), (1, 0), (0, 0)])
ob = OuterBoundaryIs(ns="")

with pytest.raises(GeometryError):
Polygon(geometry=poly, outer_boundary=ob)

def test_from_string_exterior_only(self) -> None:
"""Test exterior only."""
Expand Down
9 changes: 9 additions & 0 deletions tests/gx_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ def test_track(self) -> None:
"bbox": (0.0, 0.0, 1.0, 1.0),
"coordinates": ((0.0, 0.0), (1.0, 1.0)),
}

def test_track_etree_element(self) -> None:
g = Track()

g.etree_element()

assert g.track_items == []

def test_multitrack(self) -> None:
doc = """
Expand Down Expand Up @@ -330,6 +337,8 @@ def test_from_multilinestring(self) -> None:

mt = MultiTrack(geometry=lines, ns="")

assert mt.__bool__() is True
apurvabanka marked this conversation as resolved.
Show resolved Hide resolved

assert (
mt.to_string()
== MultiTrack(
Expand Down
Loading
Loading