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

Enforce non-CH graphs for all LocationIndex implementations #1816

Merged
merged 2 commits into from
Dec 4, 2019

Conversation

easbar
Copy link
Member

@easbar easbar commented Dec 4, 2019

These calls to CHGraph#disconnect aren't doing anything and there aren't even any shortcut edges involved in this test. This has been like this since a long time, for example already in 2015, e.g. here 7f80425. Maybe we should rename the test or remove it even (is it still testing something useful) ?

@easbar easbar added this to the 1.0 milestone Dec 4, 2019
@karussell
Copy link
Member

Yeah, very old stuff. Was introduced in #30 but at that time we had two different LocationIndex implementations. One for CH and one for none-CH graphs. So probably we do not need this anymore.

@easbar
Copy link
Member Author

easbar commented Dec 4, 2019

LocationIndexTree even throws IllegalState for CH graphs in the constructor (but the other implementations do not). Maybe we can add the same check in the others as well and delete the entire test class (LocationIndexTreeCHTest) ? Location index is not related to CH at all (anymore) ?

@easbar easbar changed the title Remove noop disconnect calls in testCHGraphBug Enforce non-CH graphs for all LocationIndex implementations Dec 4, 2019
@easbar
Copy link
Member Author

easbar commented Dec 4, 2019

Now I am throwing IllegalState for all LocationIndex implementations and deleted the LocationIndexCHTest.

@karussell
Copy link
Member

Or maybe we remove all other LocationIndex implementations or move them into the tests? I think the only used one is the full edge index for correctness tests.

@easbar
Copy link
Member Author

easbar commented Dec 4, 2019

Or maybe we remove all other LocationIndex implementations or move them into the tests? I think the only used one is the full edge index for correctness tests.

Ok go ahead if you want, for me its sufficient to get rid of the .disconnect calls for now (I cant really judge if we should keep the other location indexes or not) :)

@easbar easbar merged commit b7a9ec8 into master Dec 4, 2019
@easbar easbar deleted the noop_disconnect branch December 4, 2019 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants