Skip to content

Use the new EncodeMapTask output in the SDK and stop using resources#2091

Merged
priettt merged 1 commit intomainfrom
priettt/useNewEncodedMap
Apr 15, 2025
Merged

Use the new EncodeMapTask output in the SDK and stop using resources#2091
priettt merged 1 commit intomainfrom
priettt/useNewEncodedMap

Conversation

@priettt
Copy link
Copy Markdown
Contributor

@priettt priettt commented Apr 11, 2025

I tried to keep this small but I had to replace many parts at once so it doesn't stop working.

  • Stop looking into resources in the SDK, fetch symbols from the new instrumentedConfig field.
  • Update SymbolService tests to verify behavior of the new injected symbols.
  • Use project.afterEvaluate in AsmTaskRegistration to ensure that the AsmTask is found.
  • Register EncodeSharedObjectFilesTask instead of InjectSharedObjectFilesTask (will remove this one in a following PR).
  • Modify AndroidNdkTest: Now we're disassembling the apk and parsing the Smali to ensure the map is injected as expected. Removed resources verification.

@priettt priettt requested a review from a team as a code owner April 11, 2025 01:26
Copy link
Copy Markdown
Member

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

"${EncodeSharedObjectFilesTask.NAME}${data.name.capitalizedString()}",
EncodeSharedObjectFilesTask::class.java
) ?: return@transformClassesWith
project.afterEvaluate {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What testing have we done around this to confirm that it works?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mostly the existing tests and debugging in different setups. Any particular test you think we should add? Tests do break without this 😅

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm happy with that level of testing - the smali decompilation tests should be enough

@priettt priettt force-pushed the priettt/useNewEncodedMap branch from ab9fc42 to 4b21e14 Compare April 11, 2025 12:01
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.33%. Comparing base (b0572d4) to head (c159046).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...bracesdk/internal/ndk/symbols/SymbolServiceImpl.kt 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2091      +/-   ##
==========================================
- Coverage   85.35%   85.33%   -0.02%     
==========================================
  Files         470      470              
  Lines       10585    10578       -7     
  Branches     1564     1564              
==========================================
- Hits         9035     9027       -8     
  Misses        870      870              
- Partials      680      681       +1     
Files with missing lines Coverage Δ
...racesdk/internal/injection/NativeCoreModuleImpl.kt 97.77% <ø> (-0.05%) ⬇️
...bracesdk/internal/ndk/symbols/SymbolServiceImpl.kt 83.33% <80.00%> (-8.34%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@priettt priettt force-pushed the priettt/injectEncodedMap branch from caef597 to 8f0dacb Compare April 15, 2025 12:50
Base automatically changed from priettt/injectEncodedMap to main April 15, 2025 13:25
@priettt priettt force-pushed the priettt/useNewEncodedMap branch from 4b21e14 to c159046 Compare April 15, 2025 13:26
@github-actions
Copy link
Copy Markdown
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@priettt priettt merged commit b8a18cd into main Apr 15, 2025
10 checks passed
@priettt priettt deleted the priettt/useNewEncodedMap branch April 15, 2025 15:07
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.

2 participants