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

More tests #248

Merged
merged 11 commits into from
Aug 28, 2023
Merged

More tests #248

merged 11 commits into from
Aug 28, 2023

Conversation

vtnate
Copy link
Collaborator

@vtnate vtnate commented Apr 26, 2023

Resolves #232

Pull Request Description

We found cases where our tests weren't accounting for new potential outcomes. This makes more tests to cover those cases

Checklist (Delete lines that don't apply)

  • Unit tests have been added or updated
  • Documentation has been modified appropriately
  • All ci tests pass (green)
  • An issue has been created (which will be used for the changelog)
  • This branch is up-to-date with develop

@vtnate vtnate requested a review from kflemin April 26, 2023 16:06
@vtnate vtnate marked this pull request as ready for review April 26, 2023 16:06
@vtnate
Copy link
Collaborator Author

vtnate commented Apr 26, 2023

@kflemin Jenkins passes all the tests but fails the rake task and still sets the build to green. I'm looking forward to relying on GHA instead of Jenkins.

@vtnate
Copy link
Collaborator Author

vtnate commented Apr 26, 2023

Refactoring to not require number_of_stories (as requested in #232) would take some thought. It, along with floor_to_floor_height, is used for the shading calculations in building.rb & helper.rb. Using maximum_roof_height and/or roof_elevation would not help because those aren't available with os-hpxml or osm either.

@vtnate vtnate requested review from kflemin and removed request for kflemin August 21, 2023 20:52
@vtnate vtnate merged commit fc299b8 into develop Aug 28, 2023
@vtnate vtnate deleted the more-tests branch August 28, 2023 15:29
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.

Geojson Gem Tests
2 participants