Skip to content

Add more python integration tests + fix pre-commit ruff-format#8

Merged
jiayuasu merged 15 commits intoapache:mainfrom
petern48:more_integ_tests
Sep 2, 2025
Merged

Add more python integration tests + fix pre-commit ruff-format#8
jiayuasu merged 15 commits intoapache:mainfrom
petern48:more_integ_tests

Conversation

@petern48
Copy link
Contributor

@petern48 petern48 commented Aug 29, 2025

  • Moved st_dwithin to test_predicates
  • Add tests for st_buffer, st_asbinary, st_geo(m/g)fromwkb
  • Removed a duplicate ruff-format entry (the older version) in pre-commit that was conflicting with the other, preventing successful passing the check when they wanted different things

@petern48 petern48 changed the title Add more python integration tests Add more python integration tests + fix pre-commit ruff-format Aug 29, 2025
@petern48 petern48 requested a review from paleolimbot August 29, 2025 22:35
@petern48 petern48 marked this pull request as ready for review August 29, 2025 23:42
@jiayuasu jiayuasu requested a review from Copilot August 30, 2025 06:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive integration tests for several PostGIS/SedonaDB spatial functions and fixes a pre-commit configuration issue. The changes include moving st_dwithin tests to the appropriate predicates file and adding new tests for buffer, binary conversion, and WKB functions.

  • Reorganized st_dwithin test location from functions to predicates module
  • Added integration tests for st_buffer, st_asbinary, st_geogfromwkb, and st_geomfromwkb functions
  • Fixed duplicate ruff-format entry in pre-commit configuration that was causing conflicts

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
python/sedonadb/tests/functions/test_predicates.py Added st_dwithin test cases and imported val_or_null helper
python/sedonadb/tests/functions/test_functions.py Added new function tests (st_buffer, st_asbinary, st_astext, st_geogfromwkb, st_geomfromwkb) and removed duplicate st_dwithin tests
python/sedonadb/python/sedonadb/testing.py Enhanced testing utilities with binary data support and improved float comparison
.pre-commit-config.yaml Removed duplicate ruff-format configuration entry

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines -47 to -53
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.1.5
hooks:
- id: ruff
args: [ --fix ]
- id: ruff-format

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep ... i was left very confused why pre-commit was complaining and not making any changes until I realized there were two! 🙃

def test_st_geogfromwkb(eng, geom, expected):
eng = eng.create_or_skip()
eng.assert_query_result(
f"SELECT ST_GeogFromWKB(ST_AsBinary({geom_or_null(geom)}))", expected
Copy link
Member

Choose a reason for hiding this comment

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

This would be a less circular test if it used shapely to convert to WKB. That would also let you check flavor="iso" and flavor="extended", which both engines should accept.

It's also worth checking what happens on invalid input (we error...I'm not sure what PostGIS does).

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you! Just one nit to look into!

eng.assert_query_result(
f"SELECT ST_Area(ST_Buffer({geom_or_null(geom)}, {val_or_null(dist)}))",
expected_area,
numeric_epsilon=1e-1,
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a pretty high epsilon for things we think should be almost equal. Can this be lower?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's right ... I forgot how numbers work for a sec. This is a higher value, not a lower one. I changed it to the same as the default value.

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

@jiayuasu jiayuasu merged commit f3a0a81 into apache:main Sep 2, 2025
2 checks passed
@petern48 petern48 deleted the more_integ_tests branch September 3, 2025 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants