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

Add test for maritime_boundary #1484

Merged
merged 8 commits into from
Apr 6, 2018
Merged

Add test for maritime_boundary #1484

merged 8 commits into from
Apr 6, 2018

Conversation

nvkelso
Copy link
Member

@nvkelso nvkelso commented Apr 5, 2018

  • Update tests
  • Update data migrations
  • Update docs

@nvkelso nvkelso changed the title WIP: Create 1482-maritime_boundary-buffered_land.py WIP: Add test for maritime_boundary Apr 5, 2018
self.load_fixtures([
'https://www.openstreetmap.org/relation/148838',
'https://www.openstreetmap.org/relation/1428125',
'file://integration-test/fixtures/buffered_land/'
Copy link
Member Author

Choose a reason for hiding this comment

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

By hosting the shapefile in a buffered_land directory FixtureTest will set that as the database table name on import.

The way that the shapefile fixtures work at the moment is different from how the OSM URL fixtures work (although it would be better to load them explicitly). Moving the buffered land shapefile into the fixtures directory means it gets loaded automatically.
@zerebubuth zerebubuth force-pushed the nvkelso/buffered-land-test branch from 5db16cb to 36ad71b Compare April 6, 2018 11:01
@zerebubuth
Copy link
Member

Following 36ad71b, we now have two test failures (rather than the errors we had before). I think these failures represent the actual regressions we have in the code for dealing with maritime boundaries, so in that sense the tests are "working". The code is the next thing we need to fix.

…ously was set up.

Tightened up the filter for maritime `buffered_land` polygons, so it's perhaps a little more obvious where they come from.
@zerebubuth
Copy link
Member

After c0768c9, test failures just say AssertionError: Unknown source: tilezen.org, which is fixed in tilezen/tilequeue#330

@nvkelso nvkelso requested a review from zerebubuth April 6, 2018 17:58
@nvkelso nvkelso changed the title WIP: Add test for maritime_boundary Add test for maritime_boundary Apr 6, 2018
@nvkelso
Copy link
Member Author

nvkelso commented Apr 6, 2018

Circle is happy now! @zerebubuth, can you give a formal review, please?

# TODO: either set up new buckets, or deprecate these things.
#- ./scripts/update-integration-test-coordinates.sh
#- pip install awscli
#- aws s3 sync ~/.cache/vector-datasource/ s3://mapzen-tiles-assets/integration-test-fixtures/ --size-only --exclude="*" --include="*.geojson"
Copy link
Member Author

Choose a reason for hiding this comment

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

This one exists now:

In retrospect we should probably call the bucket tilezen-assets instead...

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a big deal to rename it. It's just a matter of updating the getting started instructions and configuration for the shapefile import/prepare processes right?

Copy link
Member

@zerebubuth zerebubuth 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 to me. I'm cool with leaving the decision about the cache bucket to a later issue. It would be nice to set it up, but I don't think it's critical.

@nvkelso nvkelso merged commit 7177f3b into master Apr 6, 2018
@nvkelso nvkelso deleted the nvkelso/buffered-land-test branch April 6, 2018 23:21
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