789 fetchnhd test cov #798
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Contributor
cristinamullin
approved these changes
Jan 28, 2026
Collaborator
cristinamullin
left a comment
cristinamullin
left a comment
There was a problem hiding this comment.
Excellent tests additions and coverage improvement!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Summary
This PR adds several tests to run against the function fetchNHD().
This function is lines ~675-1116, and current coverage was only 8.8%. This PR increases the total source file coverage from 34% to 40%, and the function coverage to >90% (see not covered).
Note: this PR shouldn't rely on code in any of the PRs pending review as the testdata was on a previously merged PR. Outside of changes to test-GeospatialFunctions.R all other changes are just the result of running the bot maintenance (hadn't been run in a bit and tests have been failing...)
Not covered (should be addressed w/ refactor first - see #805):
803-808 - When no catchments are returned (different from no results message in that it still returns empty df).
963-968 & 974-979 & 985-990 & 997-1001- What is returned with different combinations of results. Suspect this could be handled more generically, especially given the prior check if no results for specific result message.
Not covered and minor (several are just a no results message):
724 812 870-872 878-881 948-952
Pull Request Checklist
Preparation
Update your branch from the latest
developand resolve any merge conflictsBefore creating a pull request trigger the format-update GitHub Action on your branch to format the code
Documentation
Add/update inline and/or block comments to clarify complexity, context and intent
NA - Add/update function documentation (roxygen), include working examples, build docs locally using devtools::document(), and inspect added/updated help pages for content and format
NA - Add/update vignettes for corresponding changes in functionality, list these under articles in _pkgdown.yml, and ensure added/updated vignettes run and build with proper formatting locally
Maintenance & Data Refresh
Tests and checks
Add/update tests in
tests/testthatto cover changesRun
devtools::test()to verify new and existing tests passRun
devtools::check()and address any errors, warnings or notesPull Request Description
Includes a summary of the changes made
Includes relevant context/motivation
Includes links to related issues or pull requests (keywords like "Closes #issue_number" automatically close related issues when pull request is merged)
Review
Review the bot-commented coverage-report generated by test-coverage to confirm all changes are covered by tests
Review/accept any bot-suggested format changes
Request review from at least one developer team member