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

Draft: Setup testing #67

Open
wants to merge 52 commits into
base: main
Choose a base branch
from
Open

Draft: Setup testing #67

wants to merge 52 commits into from

Conversation

elbeejay
Copy link
Contributor

@elbeejay elbeejay commented Mar 24, 2024

Tests for the GOSTnets package

This PR provides both unit and integrative tests for the GOSTnets package.

In total, this PR includes 91 tests, covering 72% of the codebase (2072 lines of code).

Running the test suite

A new CI workflow (test.yml) was added to automatically run the tests on pushes and pull requests. If desired, we can work to add a "coverage badge" that displays the percentage of code covered by the test suite on the README or something along those lines.

To run the test suite locally, you can follow the instructions in the CI workflow, which are to clone the repository, and then install the [dev] version of the package: pip install .[dev]. Then you can run the test suite with the command pytest. If you are interested in the code coverage, you should use the coverage tool to run pytest: coverage run -m pytest, after the tests run, you can view coverage in the terminal with coverage report, or generate a nice HTML view with coverage html which enables you to look at the specific lines of code covered and not covered by the tests.

Unit Tests

There are 79 "unit" tests that aim to atomically test and validate the performance of individual functions. These tests sometimes involve "mocking" of functions, objects, or data to isolate the functionality of each individual function (in normal use, functions may call each other, or rely on outputs from other functions as their input).

Integrative Tests

There are 12 "integrative" tests that are more similar to example workflows and test the functions on test data. As opposed to the "unit" tests that aim to test each function in isolation, the integrative tests allow functions to call nested functions and so-on, just as they would when the package is being used normally.

Missing Tests

This PR implements tests that call 72% of the lines of code in the GOSTnets package. Lines of code that are untested include:

  • 26 lines of code in osm_parser.py:
    • The ImportError related to not having installed GDAL is untested
    • Requesting data via a proxy in download_osm() is untested
    • Handling of "oneway" tags in read_osm() is untested
    • Some other one-off lines of code covering specific cases in functions
  • 27 lines of code in load_osm.py
    • Reading of roads information from a shapefile in OSM_to_network.fetch_roads() and OSM_to_network.fetch_roads_and_ferries()
    • Some CSV reading and exception handling in OSM_to_network.initialReadIn()
    • Some one-off lines and small cases
  • 537 lines of code in core.py
    • A few cases and exceptions in node_gdf_from_graph()
    • The single edge case in edge_gdf_from_graph()
    • Alternative CRS situations in graph_nodes_intersecting_polygon() and graph_edges_intersecting_polygon()
    • Some functionality in sample_raster()
    • A number of other small cases in various functions
    • assign_traffic_times() is untested because it requires a Mapbox token (and in lieu of that I don't have my hands on sample data)
    • The weighted_origins option in calculate_OD()
    • A number of cases in simplify_junctions() and pandana_snap() (and similar functions)
    • join_networks() is currently untested as I couldn't come up with a simple case that worked there
    • update_nodes() is untested as the parameter arguments need to be revised and some input handling logic is needed to ensure the appropriate optional parameters are provided based on the method being employed
    • updated_edges() is untested because a large number of variables within the function are never defined


However given the fact that we've achieved coverage of the majority of code and functionlity, I believe it is reasonable to merge this PR in now. Some of the tests to write in the future will require detailed updates of the functions themselves, which may be best done on a single case-by-case basis to make the review of the PR easier.

This PR already does some minor editing to functions as needed due to deprecation of parameters and other changes in upstream dependencies.

@elbeejay elbeejay marked this pull request as ready for review July 20, 2024 13:02
@elbeejay elbeejay requested a review from g4brielvs July 20, 2024 13:03
@elbeejay
Copy link
Contributor Author

It's a pretty big PR so let me know if you'd like me to break it up into smaller PRs or something different to make it more digestible @g4brielvs

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.

None yet

1 participant