Clean up tests for HistoryRecordingStateChangeObserver

[dzinad]

Follow up from https://github.com/mapbox/mapbox-navigation-android/pull/6074. Comments to be addressed:

1. https://github.com/mapbox/mapbox-navigation-android/pull/6074#discussion_r927033545 - partially done, partially leave as-is
2. https://github.com/mapbox/mapbox-navigation-android/pull/6074#discussion_r927035367 - leave as-is
3. https://github.com/mapbox/mapbox-navigation-android/pull/6074#discussion_r927041682 - done
4. https://github.com/mapbox/mapbox-navigation-android/pull/6074#discussion_r927062485 - partially done, partially leave as-is
5. https://github.com/mapbox/mapbox-navigation-android/pull/6074#discussion_r927495671 - done
6. https://github.com/mapbox/mapbox-navigation-android/pull/6074#discussion_r927503161 - done
7. https://github.com/mapbox/mapbox-navigation-android/pull/6074#discussion_r927503868 - done

[dzinad]

Open for discussion. Generally is preferable having only 1 assertion per test 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=57 The problem of having multiple assertions are: It's annoying to have to look at the stacktrace to know what's failing (which assertion failed?) You don't have information about the remaining assertions (because when an assertion fails it breaks the execution of the test) That's why is preferable to have only 1 assertion per test and one test per scenario. Basically, if one of them fails you'll know at a glance what in the code might have caused that failure and in which part (class, method, etc.). I rarely write tests with multiple assertions (at least I try 😅), only if they're tearing apart the result object to verify everything e.g. if there's no equals (which is another problem although could happen if it's coming from a 3rd party library object) or if the assertions are atomic to the object (what we're asserting needs to be verified as whole). Is this one of these cases? Could we split the assertions into different tests?

In the test you are referring to (history_recording_observer_events) the whole point is to check different states and different transitions. So, for example, I have to check that if I first start trip session and then set routes, I'd transition from FreeDrive to ActiveGuidance. But in order to achieve that in a test, I'd have to first start trip session. While I would also have a test to check the transition from Idle to FreeDrive where I'd also start a trip session. So we get duplicated code. Moreover, there may be cases when only after multiple transitions a bug is reproduced. For example, we test FD -> AG, it's successful, we test AG -> FD, it's also successful, but FD -> AG -> FD fails because a state is not updated properly or sth like that. I see your point about remaining assertions, but it can be solved using SoftAssertions, for example. I also don't see why it would be too bad. You see a failed assertion, you fix that, then you run the test. If something else fails, you'll see it and fix as well.

It's annoying to have to look at the stacktrace to know what's failing (which assertion failed?)

For this we can use custom error messages that describe the failure extensively.

@Guardiola31337

[dzinad]

What about extracting this block of code into a private function and give it a name based on the comment? This way the test will be easier to read and understand and comment will become superfluous so it won't be necessary.

Not sure about that. For different states we would need different actions to start free drive. For example, for Idle it's startTripSession, for AG - set empty routes. And comments are there to describe expected state changes with some explanations why it is like that, not what I'm actually doing.

[dzinad]

Discussed internally about multiple assertions. Agreed that we would strongly prefer 1 assertion per unit test, while in instrumentation tests include several ones if necessary and more effective while trying to keep the code readable.