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.

Sign up for GitHub

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

Jump to bottom

[ready] add cucumber tests for guidance #2133

Merged
merged 1 commit into from
Apr 8, 2016
Merged

[ready] add cucumber tests for guidance #2133

merged 1 commit into from
Apr 8, 2016

Conversation

MoKob
Copy link

@MoKob MoKob commented Mar 23, 2016

Aims at implementing #2114

Requires merge of: #2135

  • merged

Requires Tests for

  • Suppressed // location that suppresses a turn
  • NewName // no turn, but name changes
  • Continue // remain on a street
  • Turn // basic turn
  • Merge // merge onto a street
  • Ramp // special turn (highway ramp exits)
  • Fork // fork road splitting up
  • EndOfRoad // T intersection
  • motorways
  • Roundabouts
  • Rotary - requires [ready] adds a distinction between roundabouts and rotaries #2135
  • fix obvious on motorways
  • unnamed fork discovery

@MoKob MoKob force-pushed the features/guidance/testing branch from 537eabb to 6b4ede4 Compare March 24, 2016 14:31
@MoKob MoKob force-pushed the features/guidance/testing branch 3 times, most recently from 2879ab5 to 4e4bc3a Compare March 30, 2016 09:34
@MoKob MoKob mentioned this pull request Mar 31, 2016
Merged
21 tasks
@MoKob MoKob force-pushed the features/guidance/testing branch from 4e4bc3a to 4255db7 Compare March 31, 2016 09:28
@MoKob MoKob added this to the OSRM 5.0 milestone Mar 31, 2016
@MoKob MoKob force-pushed the features/guidance/testing branch 2 times, most recently from 5665254 to 740a64c Compare April 1, 2016 08:33
@TheMarex TheMarex force-pushed the features/guidance/testing branch from 740a64c to ee071ea Compare April 1, 2016 18:48
@MoKob MoKob force-pushed the features/guidance/testing branch from b732815 to 083d881 Compare April 4, 2016 13:24
@MoKob MoKob mentioned this pull request Apr 4, 2016
Closed
10 tasks
@MoKob MoKob changed the title [wip] add cucumber tests for guidance add cucumber tests for guidance Apr 4, 2016
@MoKob
Copy link
Author

MoKob commented Apr 4, 2016

The pull request is both adding tests for all guidance scenarios and fixes small errors associated with it.

It moves all tests for instructions into the guidance section.
Currently, it focusses mostly on cars, even though some instructions are also tested on the bike profile.

For future test cases, we should make sure that instructions are only tested in the guidance part of the features to keep all guidance tests in a single place and focus on the paths for other instructions.

daniel-j-h
daniel-j-h reviewed Apr 4, 2016
View reviewed changes
include/extractor/guidance/toolkit.hpp
@@ -350,8 +350,9 @@ inline bool requiresNameAnnounced(const std::string &from, const std::string &to
const auto ref_begin = name.find_first_of('(');
if (ref_begin != std::string::npos)
{
out_name = name.substr(0, ref_begin);
out_ref = name.substr(ref_begin + 1, name.find_first_of(')') - 1);
out_name = name.substr(0, ref_begin-1);
Copy link
Member

Choose a reason for hiding this comment

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

This breaks if "(" is the first character?

Copy link
Author

Choose a reason for hiding this comment

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

it should theoretically work, based on the concatenation in the form of + "_(". To be on the save side, I have changed it to check for ref_begin > 0.

@MoKob MoKob force-pushed the features/guidance/testing branch from 4b61ee7 to 0ffd655 Compare April 4, 2016 15:02
@MoKob MoKob mentioned this pull request Apr 4, 2016
Closed
@MoKob MoKob force-pushed the features/guidance/testing branch 3 times, most recently from df1eb82 to 44a62bd Compare April 5, 2016 13:31
@MoKob MoKob changed the title add cucumber tests for guidance [ready] add cucumber tests for guidance Apr 5, 2016
@MoKob MoKob force-pushed the features/guidance/testing branch 2 times, most recently from a66eacb to 35097a4 Compare April 5, 2016 15:23
@TheMarex
Copy link
Member

TheMarex commented Apr 5, 2016

I have 16 failing tests here when running this locally. Debugging this revealed that osrm-extract crashes while preparing some datasets. Currently trying to figure out why.

@TheMarex
Copy link
Member

TheMarex commented Apr 5, 2016

A few tests were failing because the names were all set empty which tripped up NamesTable. The remaining tests seem to fail because of failing rotary classification. Seems like roundabout instructions are returned.

@MoKob
Copy link
Author

MoKob commented Apr 6, 2016

Can you elaborate what tests are failing, because on my side the tests run fine :(

@MoKob MoKob force-pushed the features/guidance/testing branch from 7d20b42 to 3e714a7 Compare April 6, 2016 07:26
daniel-j-h
daniel-j-h reviewed Apr 6, 2016
View reviewed changes
src/util/name_table.cpp
{
util::SimpleLogger().Write(logWARNING) << "list of street names is empty";
}
if( !name_stream )
Copy link
Member

Choose a reason for hiding this comment

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

fmt

@MoKob MoKob changed the title [ready] add cucumber tests for guidance [not-ready] add cucumber tests for guidance Apr 6, 2016
daniel-j-h
daniel-j-h reviewed Apr 6, 2016
View reviewed changes
src/extractor/guidance/turn_analysis.cpp
const double radius =
roundabout_nodes.size() >= 3
? util::coordinate_calculation::circleRadius(
getCoordinate(*node_itr++), getCoordinate(*node_itr++), getCoordinate(*node_itr++))
Copy link
Member

Choose a reason for hiding this comment

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

@MoKob just found this sequencing issue ^ capturing some resources from chat for the record

@TheMarex @danpat maybe you're interested.

@MoKob MoKob changed the title [not-ready] add cucumber tests for guidance [ready] add cucumber tests for guidance Apr 6, 2016
@MoKob MoKob force-pushed the features/guidance/testing branch from 03dc8d3 to 8559de1 Compare April 6, 2016 12:07
@MoKob
Copy link
Author

MoKob commented Apr 6, 2016

explicit coordinate extraction finally fixed the cucumber tests...

@MoKob MoKob force-pushed the features/guidance/testing branch from 8559de1 to 5b08c22 Compare April 7, 2016 08:06
@MoKob MoKob mentioned this pull request Apr 8, 2016
Merged
3 tasks
@MoKob MoKob force-pushed the features/guidance/testing branch from 5b08c22 to fa0a504 Compare April 8, 2016 08:43
@MoKob MoKob mentioned this pull request Apr 8, 2016
Closed
@TheMarex TheMarex merged commit fa0a504 into rewrite/new-api Apr 8, 2016
@TheMarex TheMarex deleted the features/guidance/testing branch April 8, 2016 09:15
@1ec5 1ec5 mentioned this pull request Mar 23, 2018
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
OSRM 5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@MoKob @TheMarex @daniel-j-h