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

Route pedestrians over highway=platform, define it as a pushing section for bikes. Unit testing considerations. #1614

Merged
merged 10 commits into from May 26, 2019

Conversation

@Anvoker
Copy link
Contributor

Anvoker commented May 10, 2019

Fixes #1580

Correct foot routing:

gh-issue-1580-foot-correct

Correct bike routing where gh prefers taking the road over entering the pushing section:

gh-issue-1580-bike-correct

Correct bike routing where an intermediate node forces gh to enter the pushing section:

gh-issue-1580-bike-pushing-waypoints-correct
(Prior to the fix, gh would absolutely refuse to path to waypoint 2)

Continue onto Bernastrasse, get off the bike
Keep left, get off the bike
Waypoint 1
Continue, get off the bike
Waypoint 2
Continue onto Helvetiaplatz, get off the bike
Turn left, get off the bike
Arrive at destination

The fix

The fix was relatively straightforward, not sure what much to say about it, just adding the correct definitions for highway=platform in foot and bike encoder classes.

Reading the documentation of Tag:highway=platform did illustrate that public_transport=platform will also probably have to be dealt with at some point.

Tests

I took the time to look at the test unit structure and think about improvements, since @karussell seemed interested in what I could do on the unit testing side, so I ended up writing the unit tests for fix in a different pattern.

I realized this should probably be in a different pull request altogether only after being three parts done. That's mea culpa. I wouldn't mind rewriting the tests in the current pattern for this PR and opening a separate one if that's needed, or just using the current pattern if the change is unwelcome. Just to be clear, I haven't touched any existing test logic or structure at all, just wrote new tests.

Unit Testing pattern

I noticed the inheritance hierarchy in the test structure and the lack of parameterized tests and I think I get why. JUnit doesn't really let you parameterize on a per method basis, just on a per class basis and that can be really clumsy. Having to deal with that and the need to share code logic from abstract to concrete can make things pretty hairy. However, one can make use of JUnit's TestRule and Parameterized to tackle both issues at once.

The two test classes I added are:

  • BikeFlagEncoderRule.java
  • BikeFlagEncoderDismountTest.java

BikeFlagEncoderRule sort of does what a before or setup annotation would do, except it encapsulates the same data that AbstractBikeFlagEncoderTester.java holds. It's more flexible in so far as we can use a Rule or ClassRule annotation to construct it once every test method run or once per class.

@ClassRule 
public static BikeFlagEncoderRule encoderRule = new BikeFlagEncoderRule(new BikeFlagEncoder());

Now the entire test class can use any data in encoderRule and encoderRule can hold all the data AbstractBikeFlagEncoder used to hold. Note that the usage of abstract is no longer needed because the constructor to the concrete implementation is called in the constructor of BikeFlagEncoderRule.

This opens the way to replacing the inheritance pattern with a composition pattern. That would allow the test logic in AbstractBikeFlagEncoder to moved into something more akin to a helper class, so the tests can be shared without needing inheritance.

Among other things, this has the significant benefit of making the usage of Parameterized easier. To deal with the difficulty of mixing disparate methods under a regime that requires that test parameterization happen class-wide, I propose using org.junit.experimental.runners.Enclosed.

Enclosed allows static nested classes in a test class to be run properly. Why do we want static nested classes? They allow us to constrain Parameterized and have it be scoped only with the methods that need that parameter list. This allows unparameterized tests to just exist at the top level and parameterized tests on the second level, grouped by their parameter list.

Right now, the way BikeFlagEncoderDismountTest.java is set up, the tests look like this:

bike-flag-encoder-dismount-test-test-explorer

tl;dr if this sounds interesting, I could make a new PR and rewrite:

  • AbstractBikeFlagEncoderTester.java
  • BikeFlagEncoderTest.java
  • MountainBikeFlagEncoderTest.java
  • RacingBikeFlagEncoderTest.java

to use a parameterized test pattern without inheritance.

@Anvoker Anvoker changed the title Bug/1580 Route pedestrians over highway=platform, define it as a pushing section for bikes. Unit testing considerations. May 10, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented May 22, 2019

Thank your for this. I will review soon :)

Copy link
Member

karussell left a comment

This looks good - thanks again!

Thanks also for the insights in the approach regarding the test rules. The reason we use this inheritance is very likely that we can easily customize on a per-method base (like you said).

Can you explain why we need the two new test classes BikeFlagEncoderRule.java and BikeFlagEncoderDismountTest.java ? I think the handling for the different surfaces should be already covered from other tests. I would prefer this new approach separate in a different issue. Maybe where it is compared to the existing code and I better understand which advantages we would get.

@Anvoker

This comment has been minimized.

Copy link
Contributor Author

Anvoker commented May 24, 2019

I would prefer this new approach separate in a different issue.

Understood, that's the best way to deal with it. I'll open a different PR for the test pattern proposal. I'll stash away the new tests and just write the tests for this bug fix in the current style for this PR, so it's kept separate. Probably this weekend.

The reason we use this inheritance is very likely that we can easily customize on a per-method base (like you said).

I thought again about this and the inheritance itself is mostly fine. I thought about how it could be replaced but it's not immediately obvious to me that the tradeoffs are worth it. Without inheritance it becomes annoying to share unit test logic from base/abstract to child/concrete classes. Using rules there might be a way to get JUnit to run the base tests on a child/concrete instance. And using the rule I defined definitely lets you get away without using virtual / override, which reduces coupling a bit. But I also found out that Rules are apparently on legacy support in JUnit5, which is something to consider. I'll take a deep dive into the API again to figure things out. This part is significantly less important than the parameterization anyway.

Can you explain why we need the two new test classes BikeFlagEncoderRule.java and BikeFlagEncoderDismountTest.java ?

BikeFlagEncoderRule.java appeared because defining a rule requires its own class. The rule makes available to the actual tests everything that they would have available if they had inherited from AbstractBikeFlagEncoderTester but without actually inheriting. So you just drop the rule in any test class you need those methods and state available.

BikeFlagEncoderDismountTest.java appeared because I used the Parameterized annotation. JUnit4 seems to be able to only parameterize on a class-wide basis and not on a per method basis. It felt very wrong to edit an existing test file and make it run with parameterize, so I made a new class.

In unit tests, I try to test something as close to a single atomic behavior as possible. In this case the behavior being tested is in what conditions does GH decide its necessary to get off the bike. Since the relevant conditions are essentially highway value, surface and whether the node is part of a cycle route or not, you can neatly separate test logic and data.

Why do any of this? Well, there's a few reasons:

The way the tests are written right now, you're essentially running the same method several times with different parameters in the same test method. The tests follow a setup - assert or setup - assert - reset pattern. The latter one can be especially prone to bugs, if you forget to reset or if previous lines mutated something. The unit tests lose their isolation. And when the test method fails you need to check at which point it failed. Using test parameterization, each instance of the test is effectively isolated, so you can't make a mistake and the test explorer readily shows you on which parameters it failed and on which it didn't, which makes diagnosing why a test failed much faster. Parameterized tests also let you add or remove cases without touching any of the test logic at all. As it is, the test data and logic are essentially mixed.

Considering JUnit5

Moving to JUnit5 would massively simply things because of the existence of @ParameterizedTest which lets you parameterize each method in part. I'm not sure what your opinion is on a JUnit5 migration, but if you're unsure or interested about it, I can look into what other advantages JUnit5 would bring and how difficult the migration would be and present that later.

At this point I'm getting a bit self conscious about looking like I'm wasting people's time with gold plating unit tests when there's possibly much more pressing features to deal with. But I think that having parameterized tests is important.

@easbar

This comment has been minimized.

Copy link
Collaborator

easbar commented May 24, 2019

For me better support for parameterized testing, e.g. on a per-method basis, would have been useful a couple of times so far. I feel solving this via inheritance sometimes makes the tests harder to understand. I would definitely be interested how JUnit5 can improve the situation and how much effort such a migration would take. Then again so far I did not run into any situation where the current Junit4 setup was a big hindrance and I really felt the need for something else.

@karussell

This comment has been minimized.

Copy link
Member

karussell commented May 24, 2019

Then again so far I did not run into any situation where the current Junit4 setup was a big hindrance and I really felt the need for something else.

Yes, I think this might be a big problem for the migration work: the pain is not big enough.

…hem using addPushingSection instead in the concrete classes.
@Anvoker Anvoker force-pushed the Anvoker:bug/1580 branch from 5376d8c to 12af959 May 25, 2019
@Anvoker

This comment has been minimized.

Copy link
Contributor Author

Anvoker commented May 25, 2019

I discarded the commits that were related to the proposed test unit pattern and wrote the rest of the required tests. This should be good to go now and I'll be writing a PR for the unit test pattern sometime next week.

I'm not sure where to discuss my findings about JUnit5 migration? The unit test proposal PR? Open a separate issue? Some other channel?

@karussell karussell added the bug label May 26, 2019
@karussell karussell added this to the 0.13 milestone May 26, 2019
Copy link
Member

karussell left a comment

Thanks, this looks good!

@karussell

This comment has been minimized.

Copy link
Member

karussell commented May 26, 2019

Feel free to open a new issue for the JUnit5 migration and if you like you can include a concrete and small case (e.g. as a link to your repo or similar) to see the advantages and maybe also disadvantages.

@karussell karussell merged commit d24173a into graphhopper:master May 26, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@karussell karussell added the osm label May 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.