Skip to content

Conversation

@dzinad
Copy link
Contributor

@dzinad dzinad commented Oct 11, 2022

Description

We don't add these permissions as a library because we ourselves don't do any network requests. Please correct me if I'm wrong @mapbox/navigation-android.
The failing test (TripServiceTest) is part of libnavigation-core module. This module or any of its dependencies does not have these permissions. It was working before because the permissions were added from mapbox-android-telemetry (removed from libnavigationmetrics in PR #6423).
I added the permissions only to androidTest Manifest because I don't think we should add them as part of the SDK for the reason described above. Any objections?

@dzinad dzinad requested a review from a team as a code owner October 11, 2022 17:32
@dzinad dzinad added the skip changelog Should not be added into version changelog. label Oct 11, 2022
@dzinad
Copy link
Contributor Author

dzinad commented Oct 11, 2022

Also what happened to the initiative of making the instrumentation tests builds required? @mapbox/navigation-android

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #6473 (a0bedf7) into main (de6914e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #6473   +/-   ##
=========================================
  Coverage     70.04%   70.04%           
  Complexity     4814     4814           
=========================================
  Files           703      703           
  Lines         27756    27756           
  Branches       3274     3274           
=========================================
  Hits          19441    19441           
  Misses         7037     7037           
  Partials       1278     1278           

@LukasPaczos
Copy link

@dzinad could you please add these permissions to the core module for now and create a ticket/PR for the Common SDK to include these permissions? Let's treat this as a workaround for the Mapbox stack as a whole, even if Nav SDK by itself does not make requests.

@LukasPaczos
Copy link

Also what happened to the initiative of making the instrumentation tests builds required?

Last we discussed it was about 2 weeks ago and the test seem to be stable since then. I'm going to go ahead and make them required.

@dzinad dzinad force-pushed the dd-fix-mockwebserver branch from dd3a9c8 to a0bedf7 Compare October 12, 2022 10:29
@dzinad
Copy link
Contributor Author

dzinad commented Oct 12, 2022

@dzinad could you please add these permissions to the core module for now and create a ticket/PR for the Common SDK to include these permissions? Let's treat this as a workaround for the Mapbox stack as a whole, even if Nav SDK by itself does not make requests.

Done. CORESDK-1386. Pablo also reported it to their Slack channel.

@dzinad dzinad merged commit be4cc59 into main Oct 12, 2022
@dzinad dzinad deleted the dd-fix-mockwebserver branch October 12, 2022 12:38
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.

2 participants