Skip to content

Conversation

@beekhc
Copy link
Contributor

@beekhc beekhc commented Dec 2, 2025

This fixes a particular way of using NSURLSession that the instrumentation was missing, where a session delegate implements only func urlSession(_:task:didFinishCollecting:). This was caught by some Honeycomb end-to-end tests that exercise URLSession in various ways. I suspect that this was missed because this method is in the super-protocol NSURLSessionTaskDelegate, rather than the NSURLSessionDataDelegate protocol.

I have verified that our own end-to-end tests fail before this change and pass after.

I've added a regression test to make sure this stays working.

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.75%. Comparing base (7bad8ae) to head (1d7d9af).
⚠️ Report is 257 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #998      +/-   ##
==========================================
- Coverage   67.89%   63.75%   -4.15%     
==========================================
  Files         344       91     -253     
  Lines       15169     6069    -9100     
==========================================
- Hits        10299     3869    -6430     
+ Misses       4870     2200    -2670     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@beekhc
Copy link
Contributor Author

beekhc commented Dec 3, 2025

I'm not sure why that codecov check is failing. I think it's incorrect. The report shows this PR removing test coverage from files I didn't touch, and I only added tests in this PR.

@bryce-b
Copy link
Member

bryce-b commented Dec 4, 2025

@beekhc I believe codecov is mad because this branch is 257 commits behind main, which isn't a problem for merge; but the tests from that time hand less coverage. I believe this can be ignored for merge.

Copy link
Member

@nachoBonafonte nachoBonafonte left a comment

Choose a reason for hiding this comment

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

Great catch!!, thanks for the fix.

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