Skip to content

updated mapbox-java to v6.9.0-beta.1#6499

Merged
LukasPaczos merged 1 commit intomainfrom
lp-mapbox-java-6.9.0-beta.1
Oct 24, 2022
Merged

updated mapbox-java to v6.9.0-beta.1#6499
LukasPaczos merged 1 commit intomainfrom
lp-mapbox-java-6.9.0-beta.1

Conversation

@LukasPaczos
Copy link
Copy Markdown

Description

@LukasPaczos LukasPaczos requested a review from a team as a code owner October 21, 2022 12:40
@LukasPaczos LukasPaczos added the skip changelog Should not be added into version changelog. label Oct 21, 2022
@LukasPaczos LukasPaczos enabled auto-merge (rebase) October 21, 2022 12:51
@LukasPaczos
Copy link
Copy Markdown
Author

With the update and introduction of mapbox/mapbox-java#1500 we're starting to run into mockk/mockk#255 whenever a toBuilder function is called (or even tried to be mocked) on a DirectionsJsonObject. Looks like something in the structure of the class with nested public abstract functions is tripping mockk up.

@dzinad
Copy link
Copy Markdown
Contributor

dzinad commented Oct 21, 2022

With the update and introduction of mapbox/mapbox-java#1500 we're starting to run into mockk/mockk#255 whenever a toBuilder function is called (or even tried to be mocked) on a DirectionsJsonObject. Looks like something in the structure of the class with nested public abstract functions is tripping mockk up.

Oooh, can we workaround the issue? Use real objects for instance. For classes like DirectionsJsonObject we usually just need some return values, no verifying or stubbing the answer in a complicated manner.
It may ask for some required properties of course but we can make a utility method for that to avoid copy-paste of unnecessary in this specific test data.

@LukasPaczos LukasPaczos force-pushed the lp-mapbox-java-6.9.0-beta.1 branch from 2b7f0c8 to 9e28101 Compare October 21, 2022 15:11
@LukasPaczos
Copy link
Copy Markdown
Author

Oooh, can we workaround the issue?

I think so, I pushed a new commit with a workaround that includes real instances and spies. Seems to work fine but definitely a little bit inconvenient.

@LukasPaczos
Copy link
Copy Markdown
Author

LukasPaczos commented Oct 21, 2022

Well, I was over-optimistic, looks like there are many more occurrences of that in the tests than the initial run uncovered.

@dzinad
Copy link
Copy Markdown
Contributor

dzinad commented Oct 24, 2022

Well, I was over-optimistic, looks like there are many more occurrences of that in the tests than the initial run uncovered.

Not too many, I've tried to fix them.

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 24, 2022

Codecov Report

Merging #6499 (51a2ed8) into main (c4b39fa) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 51a2ed8 differs from pull request most recent head 67138c7. Consider uploading reports for the commit 67138c7 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #6499   +/-   ##
=========================================
  Coverage     70.33%   70.33%           
+ Complexity     4909     4907    -2     
=========================================
  Files           717      717           
  Lines         27950    27947    -3     
  Branches       3295     3295           
=========================================
- Hits          19659    19657    -2     
+ Misses         7013     7012    -1     
  Partials       1278     1278           
Impacted Files Coverage Δ
...box/navigation/ui/voice/api/MapboxAudioGuidance.kt 79.68% <0.00%> (-0.32%) ⬇️
...n/dropin/navigationview/MapboxNavigationViewApi.kt 92.59% <0.00%> (-0.10%) ⬇️
.../com/mapbox/navigation/dropin/NavigationViewApi.kt 100.00% <0.00%> (ø)
...voice/internal/impl/MapboxAudioGuidanceServices.kt 38.88% <0.00%> (+2.04%) ⬆️

@LukasPaczos
Copy link
Copy Markdown
Author

Not too many, I've tried to fix them.

Nice, thank you! I only glanced late Friday and the output was looking intimidatingly long (nearly 50 tests failing). Let me clean up the last pieces and we can merge.

@LukasPaczos LukasPaczos force-pushed the lp-mapbox-java-6.9.0-beta.1 branch from 51a2ed8 to d831d85 Compare October 24, 2022 10:24
@LukasPaczos LukasPaczos force-pushed the lp-mapbox-java-6.9.0-beta.1 branch from d831d85 to ed71442 Compare October 24, 2022 10:56
@LukasPaczos LukasPaczos force-pushed the lp-mapbox-java-6.9.0-beta.1 branch from ed71442 to 67138c7 Compare October 24, 2022 12:28
@LukasPaczos LukasPaczos merged commit 8273101 into main Oct 24, 2022
@LukasPaczos LukasPaczos deleted the lp-mapbox-java-6.9.0-beta.1 branch October 24, 2022 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changelog Should not be added into version changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants