Closed cleder closed 1 month ago
Review changes with SemanticDiff.
Analyzed 14 of 18 files.
Overall, the semantic diff is 5% smaller than the GitHub diff.
1 files do not contain logic changes. | Filename | Status | |
---|---|---|---|
:grey_question: | .pre-commit-config.yaml | Unsupported file format | |
:grey_question: | pyproject.toml | Unsupported file format | |
:heavy_check_mark: | tests/conftest.py | Analyzed | |
:heavy_check_mark: | tests/hypothesis/__init__.py | No logic changes found | |
:heavy_check_mark: | tests/hypothesis/geometry_test.py | Analyzed | |
:heavy_check_mark: | tests/hypothesis/multi_geometry_test.py | Analyzed | |
:heavy_check_mark: | tests/geometries/boundaries_test.py | Analyzed | |
:heavy_check_mark: | tests/geometries/coordinates_test.py | Analyzed | |
:heavy_check_mark: | tests/geometries/geometry_test.py | 71.81% smaller | |
:heavy_check_mark: | tests/geometries/linearring_test.py | 92.52% smaller | |
:heavy_check_mark: | tests/geometries/linestring_test.py | Analyzed | |
:heavy_check_mark: | tests/geometries/multigeometry_test.py | 66.98% smaller | |
:heavy_check_mark: | tests/geometries/point_test.py | 72.51% smaller | |
:heavy_check_mark: | tests/geometries/polygon_test.py | Analyzed | |
:heavy_check_mark: | fastkml/features.py | Analyzed | |
:heavy_check_mark: | fastkml/geometry.py | 3.86% smaller | |
:grey_question: | docs/usage_guide.rst | Unsupported file format | |
:grey_question: | .github/workflows/run-all-tests.yml | Unsupported file format |
This pull request implements property-based testing for geometry classes using the Hypothesis library. It also includes improvements to the existing code, such as adding precision handling for coordinate string representations and implementing equality methods for geometry classes.
No diagrams generated as the changes look simple and do not need a visual representation.
Change | Details | Files |
---|---|---|
Implemented property-based testing for geometry classes |
|
tests/hypothesis/multi_geometry_test.py tests/hypothesis/geometry_test.py |
Improved coordinate string representation handling |
|
fastkml/geometry.py tests/geometries/geometry_test.py tests/geometries/multigeometry_test.py tests/geometries/point_test.py tests/geometries/polygon_test.py tests/geometries/linearring_test.py tests/geometries/linestring_test.py tests/geometries/coordinates_test.py tests/geometries/boundaries_test.py |
Implemented equality methods for geometry classes |
|
fastkml/geometry.py |
Updated CI/CD pipeline configuration |
|
.github/workflows/run-all-tests.yml .pre-commit-config.yaml |
Minor code improvements and refactoring |
|
fastkml/geometry.py fastkml/features.py |
The pull request introduces several enhancements across various files, focusing on improving testing capabilities, coverage reporting, and KML feature functionalities. Key changes include updates to GitHub workflows for stricter coverage thresholds, modifications to pre-commit hooks for Python version compatibility, and enhancements to geometry classes to support precision in KML output. New property-based tests utilizing the Hypothesis library are added for comprehensive coverage of geometry classes, ensuring their reliability through extensive automated testing.
File | Change Summary |
---|---|
.github/workflows/run-all-tests.yml |
Updated coverage threshold for cpython-lxml job to 95, added environment key to publish job, clarified publishing conditions, and included installation of pypa/build . |
.pre-commit-config.yaml |
Changed pyupgrade hook argument from --py37-plus to --py38-plus and added hypothesis to mypy hook's additional dependencies. |
fastkml/features.py |
Added default value for max_lines in Snippet , validation in Placemark for geometry types, and new attributes in NetworkLink for visibility and view behavior. |
fastkml/geometry.py |
Enhanced precision handling in coordinates_subelement , added __eq__ methods for geometry classes, introduced geometry property, and removed redundant __repr__ methods. |
pyproject.toml |
Added hypothesis to tests under optional dependencies and updated coverage report exclusions. |
tests/conftest.py |
Registered three Hypothesis profiles: exhaustive , coverage , and CI , each with specific settings for testing. |
tests/geometries/*.py |
Updated various test methods to include precision=6 in to_string method calls for consistent KML output formatting. |
tests/hypothesis/geometry_test.py |
Introduced property-based tests for geometry classes using Hypothesis, covering various geometric entities and validating their string representations. |
tests/hypothesis/multi_geometry_test.py |
Added property-based tests for MultiGeometry class, ensuring integrity through round-trip serialization and deserialization. |
fastkml
, which may relate to the changes in the fastkml/geometry.py
file in the main PR, particularly regarding the structure and management of geometry attributes.InnerBoundaryIs
and Polygon
classes in fastkml/geometry.py
, which directly relates to the changes in geometry handling in the main PR, especially concerning the management of boundaries and geometry attributes.π In the land of KML, we hop and play,
With precision and tests, we brighten the day.
From features to geometry, all in a row,
Our code now shines, with a beautiful glow!
So letβs leap with joy, for changes abound,
In the garden of code, new wonders are found! πΌ
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
**Action:** doctest-lxml (3.12) |
**Failed stage:** [test the pythoncode in the documentation](https://github.com/cleder/fastkml/actions/runs/11314562680/job/31464688276) [β] |
**Failed test name:** usage_guide.rst |
**Failure summary:**
The action failed because there were 2 test failures in the file usage_guide.rst :These failures caused the overall test suite to fail, resulting in the process exiting with code 1. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 255: ok 256: Trying: 257: fastkml.config.set_default_namespaces() 258: Expecting nothing 259: ok 260: 1 items passed all tests: 261: 10 tests in Configuration.rst 262: 10 tests in 1 items. 263: 10 passed and 0 failed. 264: Test passed. 265: 1 items had no tests: 266: HISTORY.rst 267: 0 tests in 1 items. 268: 0 passed and 0 failed. 269: Test passed. 270: 1 items had no tests: 271: contributing.rst 272: 0 tests in 1 items. 273: 0 passed and 0 failed. 274: Test passed. 275: 1 items had no tests: 276: fastkml.rst 277: 0 tests in 1 items. 278: 0 passed and 0 failed. 279: Test passed. 280: 1 items had no tests: 281: index.rst 282: 0 tests in 1 items. 283: 0 passed and 0 failed. 284: Test passed. 285: 1 items had no tests: 286: installing.rst 287: 0 tests in 1 items. 288: 0 passed and 0 failed. 289: Test passed. 290: 1 items had no tests: 291: modules.rst 292: 0 tests in 1 items. 293: 0 passed and 0 failed. ... 298: ok 299: Trying: 300: k = kml.KML() 301: Expecting nothing 302: ok 303: 1 items passed all tests: 304: 2 tests in quickstart.rst 305: 2 tests in 1 items. 306: 2 passed and 0 failed. 307: Test passed. 308: 1 items had no tests: 309: reference_guide.rst 310: 0 tests in 1 items. 311: 0 passed and 0 failed. ... 404: 405: 406: 407: 408: 409: |
Improved Quality Threshold for Tests The quality threshold for tests has been increased from 88 to 95 in the workflow. This ensures that our software is more reliable and better at detecting issues before they reach users or production.
New Environment and Permissions Configuration for Code Publishing
We have added a new environment (release
) and permissions set up in the workflow, specifically for code publishing to PyPI (Python Package Index, the Python community's software distribution platform). This allows us to more securely and efficiently distribute our software.
Updated Software Dependency
We have updated the pyupgrade
software dependency to require Python 3.8+. This ensures our software takes advantage of the latest features and improvements in this version of Python.
Added New Software Dependency for More Robust Typing
The hypothesis
dependency has been added to mypy
in the pre-commit configuration file. This improves the robustness of type checking in our code.
Set Default Value for an Attribute for Better Performance
We've set a default value of 2
for an attribute in the fastkml/features.py
file. This allows the code to function correctly even when no value is explicitly provided for this attribute.
Introduced Equality Comparison Methods for More Accurate Comparisons
We've introduced equality comparison methods (__eq__
) for Point
, LineString
, and Polygon
classes in the fastkml/geometry.py
file. These enable more accurate comparison of these classes' instances.
Simplified Coordinate Formatting
We've improved the coordinates_subelement
function to simplify the formatting of coordinates. This makes the code cleaner and easier to maintain.
Consistent Coordinate Formatting in Tests
Various test cases have been adjusted to use precision=6
for a consistent way of formatting coordinates. This brings uniformity across our tests.
Added Configurations for Exhaustive Testing
We've added configurations for hypothesis profiles in the tests/conftest.py
file. This allows our testing to be more exhaustive and comprehensive.
Added Tests to Maintain Consistent Formatting New tests have been added to ensure KML (Keyhole Markup Language) strings maintain consistent formatting with specified precision. This helps our software to produce consistently structured outputs.
Here are some key observations to aid the review process:
β±οΈ Estimated effort to review: 4 π΅π΅π΅π΅βͺ |
π§ͺ PR contains tests |
π No security concerns identified |
β‘ Recommended focus areas for review Test Coverage Ensure that the new property-based tests using Hypothesis cover all edge cases and potential issues for geometry classes. Test Complexity Review the complexity of the multi-geometry tests to ensure they are not overly complicated and maintain good performance. Code Refactoring Verify that the refactored geometry classes, especially the new equality checks and precision handling, are implemented correctly and efficiently. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Enhancement |
Add property-based tests to verify specific invariants of the MultiGeometry class___ **Consider adding property-based tests that verify specific properties or invariantsof the MultiGeometry class, such as checking if the bounding box of a MultiGeometry always contains all its constituent geometries.** [tests/hypothesis/multi_geometry_test.py [57-90]](https://github.com/cleder/fastkml/pull/364/files#diff-62429ef5316bba4d85f3d395ed68e6adaf2bfa4bc618ac4ebadc42ca78d6a568R57-R90) ```diff @given( id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), target_id=st.one_of(st.none(), st.text(ID_TEXT)), extrude=st.one_of(st.none(), st.booleans()), tessellate=st.one_of(st.none(), st.booleans()), altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), geometry=st.one_of( st.none(), multi_points(srs=epsg4326), ), ) -def test_multipoint_repr_roundtrip( +def test_multipoint_properties( id: typing.Optional[str], target_id: typing.Optional[str], extrude: typing.Optional[bool], tessellate: typing.Optional[bool], altitude_mode: typing.Optional[AltitudeMode], geometry: typing.Optional[MultiPoint], ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, target_id=target_id, extrude=extrude, tessellate=tessellate, altitude_mode=altitude_mode, geometry=geometry, ) - new_mg = eval(repr(multi_geometry), {}, eval_locals) # noqa: S307 + if geometry: + bbox = multi_geometry.bounds + for point in geometry.geoms: + assert bbox[0] <= point.x <= bbox[2] + assert bbox[1] <= point.y <= bbox[3] - assert multi_geometry == new_mg - if geometry: - assert isinstance(new_mg.geometry, MultiPoint) + assert multi_geometry.is_valid ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Adding property-based tests to verify invariants like bounding box containment can enhance the robustness of the tests by ensuring that fundamental properties of the MultiGeometry class are maintained. | 8 |
Add test cases for empty multi-geometries to ensure proper handling of edge cases___ **Consider adding a test case that specifically checks for the handling of emptygeometries (i.e., when geometry is an empty MultiPoint , MultiLineString , MultiPolygon , or GeometryCollection ). This would help ensure that the code correctly handles edge cases with empty multi-geometries.** [tests/hypothesis/multi_geometry_test.py [57-75]](https://github.com/cleder/fastkml/pull/364/files#diff-62429ef5316bba4d85f3d395ed68e6adaf2bfa4bc618ac4ebadc42ca78d6a568R57-R75) ```diff @given( id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), target_id=st.one_of(st.none(), st.text(ID_TEXT)), extrude=st.one_of(st.none(), st.booleans()), tessellate=st.one_of(st.none(), st.booleans()), altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), geometry=st.one_of( st.none(), multi_points(srs=epsg4326), + st.just(MultiPoint([])), ), ) def test_multipoint_repr_roundtrip( id: typing.Optional[str], target_id: typing.Optional[str], extrude: typing.Optional[bool], tessellate: typing.Optional[bool], altitude_mode: typing.Optional[AltitudeMode], geometry: typing.Optional[MultiPoint], ) -> None: ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Adding test cases for empty geometries is a valuable enhancement that ensures edge cases are handled correctly, potentially catching bugs related to empty multi-geometries. | 7 | |
Add a step to run exhaustive tests periodically in the CI pipeline___ **Consider adding a step to run tests with the 'exhaustive' Hypothesis profile. Thiswill ensure that the extensive test suite is run periodically, potentially catching edge cases that might be missed in regular CI runs.** [.github/workflows/run-all-tests.yml [45-46]](https://github.com/cleder/fastkml/pull/364/files#diff-f715a403191ed7bfccfd1db9e835d484432652b18d6bb7da5fadfb0d6202a4bcR45-R46) ```diff - name: Test with pytest run: | pytest tests --cov=fastkml --cov=tests --cov-fail-under=95 --cov-report=xml +- name: Run exhaustive tests + if: github.event_name == 'schedule' + run: | + pytest tests --hypothesis-profile=exhaustive ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Adding a step to run exhaustive tests periodically can help catch edge cases that might be missed in regular CI runs, improving the robustness of the test suite. This is a valuable enhancement to the CI pipeline. | 7 | |
β Parameterize tests for different multi-geometry types to reduce code duplication and improve test maintainability___Suggestion Impact:The commit refactored the tests by using a common geometry strategy with the `partial` function, which aligns with the suggestion's goal of reducing code duplication and improving maintainability for different multi-geometry types. code diff: ```diff -@given( +common_geometry = partial( + given, id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), target_id=st.one_of(st.none(), st.text(ID_TEXT)), extrude=st.one_of(st.none(), st.booleans()), tessellate=st.one_of(st.none(), st.booleans()), altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +) + + +def _test_repr_roundtrip( + geometry: fastkml.geometry.MultiGeometry, + cls: type[MultiPoint | MultiLineString | MultiPolygon | GeometryCollection], +) -> None: + new_g = eval(repr(geometry), {}, eval_locals) # noqa: S307 + + assert geometry == new_g + if geometry: + assert type(new_g.geometry) is cls + + +def _test_geometry_str_roundtrip( + geometry: fastkml.geometry.MultiGeometry, + cls: type[MultiPoint | MultiLineString | MultiPolygon], +) -> None: + new_g = fastkml.geometry.MultiGeometry.class_from_string(geometry.to_string()) + + assert geometry.to_string() == new_g.to_string() + assert geometry == new_g + if geometry: + assert type(new_g.geometry) is cls + + +@common_geometry( geometry=st.one_of( st.none(), multi_points(srs=epsg4326), ), ) def test_multipoint_repr_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPoint], -) -> None: - multi_geometry = fastkml.geometry.MultiGeometry( - id=id, - target_id=target_id, - extrude=extrude, - tessellate=tessellate, - altitude_mode=altitude_mode, - geometry=geometry, - ) - - new_mg = eval(repr(multi_geometry), {}, eval_locals) # noqa: S307 - - assert multi_geometry == new_mg - if geometry: - assert isinstance(new_mg.geometry, MultiPoint) - - -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPoint | None, +) -> None: + multi_geometry = fastkml.geometry.MultiGeometry( + id=id, + target_id=target_id, + extrude=extrude, + tessellate=tessellate, + altitude_mode=altitude_mode, + geometry=geometry, + ) + + _test_repr_roundtrip(multi_geometry, MultiPoint) + + +@common_geometry( geometry=st.one_of( st.none(), multi_points(srs=epsg4326), ), ) def test_multipoint_str_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPoint], -) -> None: - multi_geometry = fastkml.geometry.MultiGeometry( - id=id, - target_id=target_id, - extrude=extrude, - tessellate=tessellate, - altitude_mode=altitude_mode, - geometry=geometry, - ) - - new_mg = fastkml.geometry.MultiGeometry.class_from_string( - multi_geometry.to_string(), - ) - - assert multi_geometry.to_string() == new_mg.to_string() - assert multi_geometry == new_mg - if geometry: - assert isinstance(new_mg.geometry, MultiPoint) - - -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPoint | None, +) -> None: + multi_geometry = fastkml.geometry.MultiGeometry( + id=id, + target_id=target_id, + extrude=extrude, + tessellate=tessellate, + altitude_mode=altitude_mode, + geometry=geometry, + ) + + _test_geometry_str_roundtrip(multi_geometry, MultiPoint) + + +@common_geometry( geometry=st.one_of( st.none(), multi_points(srs=epsg4326), ), ) def test_multipoint_str_roundtrip_terse( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPoint], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPoint | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -166,24 +174,19 @@ assert isinstance(new_mg.geometry, MultiPoint) -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), multi_points(srs=epsg4326), ), ) def test_multipoint_str_roundtrip_verbose( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPoint], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPoint | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -205,97 +208,71 @@ assert isinstance(new_mg.geometry, MultiPoint) -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), multi_line_strings(srs=epsg4326), ), ) def test_multilinestring_repr_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiLineString], -) -> None: - multi_geometry = fastkml.geometry.MultiGeometry( - id=id, - target_id=target_id, - extrude=extrude, - tessellate=tessellate, - altitude_mode=altitude_mode, - geometry=geometry, - ) - - new_mg = eval(repr(multi_geometry), {}, eval_locals) # noqa: S307 - - assert multi_geometry == new_mg - if geometry: - assert isinstance(new_mg.geometry, MultiLineString) - - -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiLineString | None, +) -> None: + multi_geometry = fastkml.geometry.MultiGeometry( + id=id, + target_id=target_id, + extrude=extrude, + tessellate=tessellate, + altitude_mode=altitude_mode, + geometry=geometry, + ) + + _test_repr_roundtrip(multi_geometry, MultiLineString) + + +@common_geometry( geometry=st.one_of( st.none(), multi_line_strings(srs=epsg4326), ), ) def test_multilinestring_str_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiLineString], -) -> None: - multi_geometry = fastkml.geometry.MultiGeometry( - id=id, - target_id=target_id, - extrude=extrude, - tessellate=tessellate, - altitude_mode=altitude_mode, - geometry=geometry, - ) - - new_mg = fastkml.geometry.MultiGeometry.class_from_string( - multi_geometry.to_string(), - ) - - assert multi_geometry.to_string() == new_mg.to_string() - assert multi_geometry == new_mg - if geometry: - assert isinstance(new_mg.geometry, MultiLineString) - - -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiLineString | None, +) -> None: + multi_geometry = fastkml.geometry.MultiGeometry( + id=id, + target_id=target_id, + extrude=extrude, + tessellate=tessellate, + altitude_mode=altitude_mode, + geometry=geometry, + ) + + _test_geometry_str_roundtrip(multi_geometry, MultiLineString) + + +@common_geometry( geometry=st.one_of( st.none(), multi_line_strings(srs=epsg4326), ), ) def test_multilinestring_str_roundtrip_terse( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiLineString], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiLineString | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -317,24 +294,19 @@ assert isinstance(new_mg.geometry, MultiLineString) -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), multi_line_strings(srs=epsg4326), ), ) def test_multilinestring_str_roundtrip_verbose( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiLineString], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiLineString | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -356,97 +328,71 @@ assert isinstance(new_mg.geometry, MultiLineString) -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), multi_polygons(srs=epsg4326), ), ) def test_multipolygon_repr_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPolygon], -) -> None: - multi_geometry = fastkml.geometry.MultiGeometry( - id=id, - target_id=target_id, - extrude=extrude, - tessellate=tessellate, - altitude_mode=altitude_mode, - geometry=geometry, - ) - - new_mg = eval(repr(multi_geometry), {}, eval_locals) # noqa: S307 - - assert multi_geometry == new_mg - if geometry: - assert isinstance(new_mg.geometry, MultiPolygon) - - -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPolygon | None, +) -> None: + multi_geometry = fastkml.geometry.MultiGeometry( + id=id, + target_id=target_id, + extrude=extrude, + tessellate=tessellate, + altitude_mode=altitude_mode, + geometry=geometry, + ) + + _test_repr_roundtrip(multi_geometry, MultiPolygon) + + +@common_geometry( geometry=st.one_of( st.none(), multi_polygons(srs=epsg4326), ), ) def test_multipolygon_str_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPolygon], -) -> None: - multi_geometry = fastkml.geometry.MultiGeometry( - id=id, - target_id=target_id, - extrude=extrude, - tessellate=tessellate, - altitude_mode=altitude_mode, - geometry=geometry, - ) - - new_mg = fastkml.geometry.MultiGeometry.class_from_string( - multi_geometry.to_string(), - ) - - assert multi_geometry.to_string() == new_mg.to_string() - assert multi_geometry == new_mg - if geometry: - assert isinstance(new_mg.geometry, MultiPolygon) - - -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPolygon | None, +) -> None: + multi_geometry = fastkml.geometry.MultiGeometry( + id=id, + target_id=target_id, + extrude=extrude, + tessellate=tessellate, + altitude_mode=altitude_mode, + geometry=geometry, + ) + + _test_geometry_str_roundtrip(multi_geometry, MultiPolygon) + + +@common_geometry( geometry=st.one_of( st.none(), multi_polygons(srs=epsg4326), ), ) def test_multipolygon_str_roundtrip_terse( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPolygon], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPolygon | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -468,24 +414,19 @@ assert isinstance(new_mg.geometry, MultiPolygon) -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), multi_polygons(srs=epsg4326), ), ) def test_multipolygon_str_roundtrip_verbose( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPolygon], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: MultiPolygon | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -507,24 +448,19 @@ assert isinstance(new_mg.geometry, MultiPolygon) -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), geometry_collections(srs=epsg4326), ), ) def test_geometrycollection_repr_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[GeometryCollection], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: GeometryCollection | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -547,24 +483,19 @@ assert not new_mg -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), geometry_collections(srs=epsg4326), ), ) def test_geometrycollection_str_roundtrip( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[GeometryCollection], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: GeometryCollection | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -588,24 +519,19 @@ assert not new_mg -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), geometry_collections(srs=epsg4326), ), ) def test_geometrycollection_str_roundtrip_terse( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[GeometryCollection], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: GeometryCollection | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, @@ -629,24 +555,19 @@ assert not new_mg -@given( - id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), - target_id=st.one_of(st.none(), st.text(ID_TEXT)), - extrude=st.one_of(st.none(), st.booleans()), - tessellate=st.one_of(st.none(), st.booleans()), - altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), +@common_geometry( geometry=st.one_of( st.none(), geometry_collections(srs=epsg4326), ), ) def test_geometrycollection_str_roundtrip_verbose( - id: typing.Optional[str], - target_id: typing.Optional[str], - extrude: typing.Optional[bool], - tessellate: typing.Optional[bool], - altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[GeometryCollection], + id: str | None, + target_id: str | None, + extrude: bool | None, + tessellate: bool | None, + altitude_mode: AltitudeMode | None, + geometry: GeometryCollection | None, ) -> None: multi_geometry = fastkml.geometry.MultiGeometry( id=id, ```MultiLineString, MultiPolygon, GeometryCollection) to reduce code duplication and improve maintainability.** [tests/hypothesis/multi_geometry_test.py [57-75]](https://github.com/cleder/fastkml/pull/364/files#diff-62429ef5316bba4d85f3d395ed68e6adaf2bfa4bc618ac4ebadc42ca78d6a568R57-R75) ```diff +@pytest.mark.parametrize("geometry_type, geometry_strategy", [ + (MultiPoint, multi_points(srs=epsg4326)), + (MultiLineString, multi_line_strings(srs=epsg4326)), + (MultiPolygon, multi_polygons(srs=epsg4326)), + (GeometryCollection, geometry_collections(srs=epsg4326)), +]) @given( id=st.one_of(st.none(), st.text(alphabet=ID_TEXT)), target_id=st.one_of(st.none(), st.text(ID_TEXT)), extrude=st.one_of(st.none(), st.booleans()), tessellate=st.one_of(st.none(), st.booleans()), altitude_mode=st.one_of(st.none(), st.sampled_from(AltitudeMode)), - geometry=st.one_of( - st.none(), - multi_points(srs=epsg4326), - ), ) -def test_multipoint_repr_roundtrip( +def test_multi_geometry_repr_roundtrip( + geometry_type, + geometry_strategy, id: typing.Optional[str], target_id: typing.Optional[str], extrude: typing.Optional[bool], tessellate: typing.Optional[bool], altitude_mode: typing.Optional[AltitudeMode], - geometry: typing.Optional[MultiPoint], ) -> None: + geometry = st.one_of(st.none(), geometry_strategy).example() ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Parameterizing tests can significantly reduce code duplication and improve maintainability, making it easier to manage and extend tests for different geometry types. | 6 | |
Enhance test coverage by using a more sophisticated strategy for generating geometry objects___ **Consider using a custom composite strategy that combines different geometry typesinstead of using st.one_of() with st.none() for the geometry parameter. This would allow for more focused testing of non-null geometries and potentially uncover edge cases.** [tests/hypothesis/multi_geometry_test.py [64-66]](https://github.com/cleder/fastkml/pull/364/files#diff-62429ef5316bba4d85f3d395ed68e6adaf2bfa4bc618ac4ebadc42ca78d6a568R64-R66) ```diff geometry=st.one_of( st.none(), - multi_points(srs=epsg4326), + st.builds( + fastkml.geometry.MultiGeometry, + geometry=multi_points(srs=epsg4326) + ), ), ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 5Why: The suggestion proposes using a custom composite strategy for generating geometry objects, which could enhance test coverage by focusing on non-null geometries. However, the improvement is moderate as the existing strategy already covers a range of cases. | 5 |
π‘ Need additional feedback ? start a PR chat
Preparing review...
Preparing review...
Failed to generate code suggestions for PR
Attention: Patch coverage is 97.91045%
with 7 lines
in your changes missing coverage. Please review.
Project coverage is 98.12%. Comparing base (
b9812da
) to head (1142a1e
). Report is 17 commits behind head on develop.
Files with missing lines | Patch % | Lines |
---|---|---|
fastkml/geometry.py | 58.82% | 3 Missing and 4 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Preparing review...
Preparing review...
User description
PR Type
Tests, Enhancement
Description
Changes walkthrough π
2 files
features.py
Add default value for `max_lines` attribute
fastkml/features.py - Added a default value for `max_lines` attribute.
geometry.py
Refactor geometry classes and add equality checks
fastkml/geometry.py
xml_attrs
for attribute management.11 files
conftest.py
Configure Hypothesis profiles for testing
tests/conftest.py - Configured Hypothesis profiles for testing.
boundaries_test.py
Update boundary tests with precision parameter
tests/geometries/boundaries_test.py - Updated tests to include precision parameter.
coordinates_test.py
Update coordinate tests with precision parameter
tests/geometries/coordinates_test.py - Updated coordinate tests to include precision parameter.
geometry_test.py
Update geometry tests with precision parameter
tests/geometries/geometry_test.py - Updated geometry tests to include precision parameter.
linearring_test.py
Update LinearRing tests with precision parameter
tests/geometries/linearring_test.py - Updated LinearRing tests to include precision parameter.
linestring_test.py
Update LineString tests with precision parameter
tests/geometries/linestring_test.py - Updated LineString tests to include precision parameter.
multigeometry_test.py
Update MultiGeometry tests with precision parameter
tests/geometries/multigeometry_test.py - Updated MultiGeometry tests to include precision parameter.
point_test.py
Update Point tests with precision parameter
tests/geometries/point_test.py - Updated Point tests to include precision parameter.
polygon_test.py
Update Polygon tests with precision parameter
tests/geometries/polygon_test.py - Updated Polygon tests to include precision parameter.
geometry_test.py
Add property-based tests for geometry classes
tests/hypothesis/geometry_test.py - Added property-based tests for geometry classes using Hypothesis.
multi_geometry_test.py
Add property-based tests for multi-geometry classes
tests/hypothesis/multi_geometry_test.py
Hypothesis.
2 files
run-all-tests.yml
Update test workflow and coverage threshold
.github/workflows/run-all-tests.yml
.pre-commit-config.yaml
Update pre-commit configuration for Python and dependencies
.pre-commit-config.yaml
1 files
pyproject.toml
Update test dependencies and coverage configuration
pyproject.toml
Summary by Sourcery
Enhance geometry handling by adding equality checks and refactoring coordinate precision management. Introduce property-based testing with Hypothesis to improve test coverage and robustness. Update CI and deployment configurations to enforce stricter code coverage and secure publishing processes.
New Features:
Enhancements:
Build:
CI:
Deployment:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores