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

- Refactor test_basemap.py and add additional boundary handler tests #280

Merged
merged 7 commits into from
Aug 19, 2024

Conversation

RonaldRonnie
Copy link
Contributor

setup.

  • Added GeoJSON boundary setup and included
    BytesIO.
  • Expanded test_create to include tile
    generation for levels 8 to 12.
  • Enhanced PMTile validation with specific zoom
    level assertion.
  • Removed test_init_with_bytesio in
    TestBoundaryHandlerFactory due to an
    AttributeError.
  • Added tests for bounding box creation in
    BytesIOBoundaryHandler and
    StringBoundaryHandler classes.
  • Included validation tests for invalid and empty
    boundary strings.
  • Updated test methods to improve clarity and
    coverage.

@RonaldRonnie RonaldRonnie changed the title - Introduced setup_boundary fixture for test - Refactor test_basemap.py and add additional boundary handler tests Aug 1, 2024
@spwoodcock spwoodcock self-requested a review August 5, 2024 16:47
@spwoodcock
Copy link
Member

Thanks @RonaldRonnie !

Sorry for the delay in review - I will check the code over now 👍

Related to #203

Copy link
Member

@spwoodcock spwoodcock left a comment

Choose a reason for hiding this comment

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

A good first attempt for sure!

Nice work - thanks for the contribution 🙏

Could you possibly address the small fixes I suggested, then I can merge in the code after testing 😄

.vscode/settings.json Outdated Show resolved Hide resolved
tests/test_basemap.py Show resolved Hide resolved
tests/test_basemap.py Outdated Show resolved Hide resolved
tests/test_basemap.py Outdated Show resolved Hide resolved
tests/test_basemap.py Show resolved Hide resolved
tests/test_basemap.py Show resolved Hide resolved
tests/testdata/test.csv Show resolved Hide resolved
tests/test_basemap.py Show resolved Hide resolved
@spwoodcock
Copy link
Member

Also, apologies but the test_basemap.py file is out of sync with the main branch.

Could you merge in the main branch and resolve conflicts please?

@RonaldRonnie
Copy link
Contributor Author

Thank you @spwoodcock for the feedback. So should I first work again on the tests? any advice about this

@spwoodcock
Copy link
Member

For sure!

If you resolved all the issues and push your latest code, I can review it again and merge 😄

@RonaldRonnie
Copy link
Contributor Author

Thank you @spwoodcock , do i have to first close the PR here and the make another one?

Copy link
Member

@spwoodcock spwoodcock left a comment

Choose a reason for hiding this comment

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

Looks good! I just need to run the tests to verify when on my laptop 👍

But looks ready to merge!

@RonaldRonnie
Copy link
Contributor Author

I will be pleased for the feedback

RonaldRonnie and others added 6 commits August 19, 2024 12:42
setup.
- Added GeoJSON boundary setup and included
BytesIO.
- Expanded `test_create` to include tile
generation for levels 8 to 12.
- Enhanced PMTile validation with specific zoom
 level assertion.
- Removed `test_init_with_bytesio` in
`TestBoundaryHandlerFactory` due to an
 AttributeError.
- Added tests for bounding box creation in
`BytesIOBoundaryHandler` and
`StringBoundaryHandler` classes.
- Included validation tests for invalid and empty
 boundary strings.
- Updated test methods to improve clarity and
coverage.
This update has been made to remove the useless / unnecessary code tests
This update (fix) address and has been made to after removing the unwanted /useless or unnecessary tests from the PR
@spwoodcock
Copy link
Member

Sorry it took so long to test this!

Everything works great 🙏
Nice work 😄

@spwoodcock spwoodcock merged commit 12c0f41 into hotosm:main Aug 19, 2024
2 of 3 checks passed
@RonaldRonnie
Copy link
Contributor Author

RonaldRonnie commented Aug 20, 2024 via email

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.

2 participants