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

Fix int_ref processing when building ways #3446

Merged
merged 6 commits into from
Dec 8, 2021

Conversation

mandeepsandhu
Copy link
Contributor

Issue

int_refs were not being processed when use_direction_on_ways_ was not set. This is a bug as int_ref should always be processed irrespective of that setting.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

kevinkreiser
kevinkreiser previously approved these changes Dec 7, 2021
Copy link
Member

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

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

Looks right to me. Why was that in there in the first place?

Copy link
Member

@dgearhart dgearhart left a comment

Choose a reason for hiding this comment

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

build failures

@mandeepsandhu
Copy link
Contributor Author

Looks right to me. Why was that in there in the first place?

https://github.com/valhalla/valhalla/pull/3446/files#diff-5fcdff37eb551f814e2a44f24f9c6bcc37856bb8dadbbcb59e93d173e46c1132L1903 has been there since #2285 I beleive.

@mandeepsandhu
Copy link
Contributor Author

Fix for the remaining CI error: #3450

@mandeepsandhu
Copy link
Contributor Author

test_create_extracts fails with this change. The exact error is:

/usr/bin/python3.8 -m unittest discover -s /root/project/test/scripts -v
test_add_leaf_types (test_valhalla_build_config.TestBuildConfig) ... ok
test_nested_config (test_valhalla_build_config.TestBuildConfig)
tests if we get the values of nested dicts ... ok
test_override_config (test_valhalla_build_config.TestBuildConfig)
tests end to end if the arg parsing is working ... ok
test_get_tiles_with_bbox (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_get_tiles_with_geojson (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_get_tiles_with_graph (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_create_extracts (test_valhalla_build_extract.TestBuildExtract) ... FAIL

======================================================================
FAIL: test_create_extracts (test_valhalla_build_extract.TestBuildExtract)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/project/test/scripts/test_valhalla_build_extract.py", line 26, in test_create_extracts
    self.check_tar(EXTRACT_PATH, exp_tuples, tile_count * INDEX_BIN_SIZE)
  File "/root/project/test/scripts/test_valhalla_build_extract.py", line 36, in check_tar
    self.assertIn(t, exp_tuples)
AssertionError: (2560, 25568, 290368) not found in ((2560, 25568, 289288), (293888, 410441, 658856), (954368, 6549282, 6051320))

----------------------------------------------------------------------
Ran 7 tests in 0.101s

FAILED (failures=1)

@gknisely could it have been due to int_ref's being processed now?

@nilsnolde do you know how the values expected in the test were constructed?

@gknisely
Copy link
Member

gknisely commented Dec 8, 2021

test_create_extracts fails with this change. The exact error is:

/usr/bin/python3.8 -m unittest discover -s /root/project/test/scripts -v
test_add_leaf_types (test_valhalla_build_config.TestBuildConfig) ... ok
test_nested_config (test_valhalla_build_config.TestBuildConfig)
tests if we get the values of nested dicts ... ok
test_override_config (test_valhalla_build_config.TestBuildConfig)
tests end to end if the arg parsing is working ... ok
test_get_tiles_with_bbox (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_get_tiles_with_geojson (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_get_tiles_with_graph (test_valhalla_build_elevation.TestBuildElevation) ... ok
test_create_extracts (test_valhalla_build_extract.TestBuildExtract) ... FAIL

======================================================================
FAIL: test_create_extracts (test_valhalla_build_extract.TestBuildExtract)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/project/test/scripts/test_valhalla_build_extract.py", line 26, in test_create_extracts
    self.check_tar(EXTRACT_PATH, exp_tuples, tile_count * INDEX_BIN_SIZE)
  File "/root/project/test/scripts/test_valhalla_build_extract.py", line 36, in check_tar
    self.assertIn(t, exp_tuples)
AssertionError: (2560, 25568, 290368) not found in ((2560, 25568, 289288), (293888, 410441, 658856), (954368, 6549282, 6051320))

----------------------------------------------------------------------
Ran 7 tests in 0.101s

FAILED (failures=1)

@gknisely could it have been due to int_ref's being processed now?

@nilsnolde do you know how the values expected in the test were constructed?

@mandeepsandhu yes. update the test please

Copy link
Member

@dgearhart dgearhart left a comment

Choose a reason for hiding this comment

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

🚢 thanks @mandeepsandhu

@mandeepsandhu mandeepsandhu merged commit 7a0cb63 into master Dec 8, 2021
@nilsnolde nilsnolde deleted the fix_int_ref_processing branch February 24, 2024 15:04
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

4 participants