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 simulated speed label in NavigationViewController #49

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

michaelkirk
Copy link
Collaborator

@michaelkirk michaelkirk commented May 31, 2024

Description

For testing purposes, there is a SimulatedLocationManager. You give it a route and it updates your "current location" to be along that route.

To speed up the testing cycle, you can set a speed multiplier on the SimulatedLocationManager so that the route is travelled much faster than what it would be in realtime.

When using the SimulatedLocationManager the navigation view controller shows what the speed multiplier is. However, it used to always say "Simulating Navigation at 1x" regardless of what you'd set the speed multiplier to.

before
Screenshot 2024-05-31 at 10 44 26

Now it properly reflects the specified speed
after
Screenshot 2024-05-31 at 10 44 02

Copy link
Collaborator

@hactar hactar left a comment

Choose a reason for hiding this comment

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

Noticed this too, and the fix looks good, but one of the tests now fails cause something is too slow? Test issue?

Also @boldtrn usually would like people to add info about the PR to the changelog file ☺️

@michaelkirk michaelkirk force-pushed the mkirk/fix-simulated-speed-label branch from 9a4ae59 to 9f6c194 Compare May 31, 2024 19:02
@michaelkirk
Copy link
Collaborator Author

michaelkirk commented May 31, 2024

Noticed this too, and the fix looks good, but one of the tests now fails cause something is too slow? Test issue?

I just re-ran them and they passed. It seems to be a flaky benchmark. The bench in question seems to be a small total cost (3 ms), so a small absolute deviation is a large relative deviation. Maybe we should update the baselines if this occurs again?

Also @boldtrn usually would like people to add info about the PR to the changelog file ☺️

Thanks for the reminder. Updated!

@michaelkirk
Copy link
Collaborator Author

I think I misread the test failure. The benchmark was marked as slower, but it didn't cause the tests to fail.

The failure is a cryptic:

Notice: Tests Passed: 0 failed, 0 skipped, 43 total (8.958 seconds)
Error: Process completed with exit code 65.

So tests passed, but non-zero exit code from the test runner.

The runs that fail seem to run much slower (e.g. more than 20+ minutes vs. the passing tests 5-10min)

Re-starting the action succeeded. I'm not sure what to make of it.

Copy link
Collaborator

@hactar hactar left a comment

Choose a reason for hiding this comment

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

I approve this change - but I'm not going to merge it - someone with more knowledge about the tests should take a look and confirm that we're ok ☺️

@boldtrn
Copy link
Collaborator

boldtrn commented Jun 3, 2024

Also @boldtrn usually would like people to add info about the PR to the changelog file ☺️

Just to clarify, while it certainly doesn't hurt, IMHO we don't need to document tiny changes like this. But especially if we remove code, change APIs, or change anything else, that might break / interrupt other implementers, we should really add this to the changelog.

Thanks for working on this @michaelkirk, I think we can merge this :)

Thanks for the review @hactar 👍

@boldtrn boldtrn merged commit 0c7a0f6 into maplibre:main Jun 3, 2024
2 checks passed
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.

3 participants