-
Notifications
You must be signed in to change notification settings - Fork 92
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
Changes from 11 commits
056095a
84e1787
ad4d00c
d5bf4c9
a101c82
f0eaa11
4b8a9ed
bf96ebf
8e48250
040d092
589a00f
712587e
c106b2c
349436a
84e8bb9
5ebf637
5ad43b6
200bcb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,13 +17,14 @@ | |||||||||||||||||||||||||||||||||||||||||
"""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): | ||||||||||||||||||||||||||||||||||||||||||
"""Test with the standard library.""" | ||||||||||||||||||||||||||||||||||||||||||
"""Test with the standard library.""" | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
def test_document_boolean_visibility(self) -> None: | ||||||||||||||||||||||||||||||||||||||||||
doc = """<kml xmlns="http://www.opengis.net/kml/2.2"> | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||
with pytest.raises(ValueError): | ||||||||||||||||||||||||||||||||||||||||||
container.append(container) | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_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): | ||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. implemented There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def test_read_outer_boundary(self) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Test the from_string method.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 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:
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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. implemented There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def test_read_inner_boundary_multiple_linestrings(self) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
There was a problem hiding this comment.
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 returnsNone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed