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

Bug 1721193 - Remove reliance on Operation for uploading tasks in the background #1783

Merged
merged 6 commits into from
Sep 29, 2021

Conversation

travis79
Copy link
Member

This relies on the background operation capabilities that are built into URLSession, which we were already using wrapped in an Operation. This removes the wrapping, using just the URLOperation capabilities.

@auto-assign auto-assign bot requested a review from mdboom September 10, 2021 20:36
@travis79 travis79 force-pushed the playing-with-ios-fire branch from a560a87 to 1dd23be Compare September 10, 2021 20:49
@badboy
Copy link
Member

badboy commented Sep 13, 2021

I assume this is for bug 1721193?

@badboy
Copy link
Member

badboy commented Sep 13, 2021

This certainly simplifies things.
I guess the tests all fail because the upload isn't scheduled right away anymore. Maybe there's something we can do to trigger them in test mode immediately?

@travis79
Copy link
Member Author

Yeah, the tests were being a pain on Friday so I opted to look into them with fresh eyes today.

@travis79
Copy link
Member Author

I assume this is for bug 1721193?

Yes, I forget we have the bug linking thing if I name my PR's correctly.

@badboy badboy changed the title Remove reliance on Operation for uploading tasks in the background Bug 1721193 - Remove reliance on Operation for uploading tasks in the background Sep 13, 2021
@mdboom mdboom requested a review from badboy September 13, 2021 14:12
@badboy badboy removed the request for review from mdboom September 14, 2021 10:08
@travis79 travis79 force-pushed the playing-with-ios-fire branch 3 times, most recently from 3465972 to 1c077fb Compare September 15, 2021 14:12
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #1783 (a5001e5) into main (2791d63) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1783   +/-   ##
=======================================
  Coverage   29.41%   29.41%           
=======================================
  Files           1        1           
  Lines          34       34           
=======================================
  Hits           10       10           
  Misses         24       24           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9923897...a5001e5. Read the comment docs.

Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Lots of questions inline.
Code itself looks good, it's mostly about our testing strategy and why certain things are happening.

CHANGELOG.md Outdated Show resolved Hide resolved
glean-core/ios/Glean/Net/HttpPingUploader.swift Outdated Show resolved Hide resolved
glean-core/ios/Glean/Net/HttpPingUploader.swift Outdated Show resolved Hide resolved
glean-core/ios/GleanTests/GleanTests.swift Outdated Show resolved Hide resolved
@travis79 travis79 force-pushed the playing-with-ios-fire branch from cb2c00b to ed450c5 Compare September 16, 2021 21:16
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Re-raising a couple points for visibility.
I merged #1791, I would appreciate if you rebase and include the changes to the URL metric type tests.
Switching to a method resetGleanDiscardingInitialPings is a good idea!

CHANGELOG.md Outdated Show resolved Hide resolved
glean-core/ios/Glean/Net/HttpPingUploader.swift Outdated Show resolved Hide resolved
glean-core/ios/Glean/Net/HttpPingUploader.swift Outdated Show resolved Hide resolved
glean-core/ios/GleanTests/GleanTests.swift Outdated Show resolved Hide resolved
glean-core/ios/GleanTests/GleanTests.swift Outdated Show resolved Hide resolved
This relies on the background operation capabilities that are built into URLSession, which we were already using wrapped in an `Operation`. This removes the wrapping, using just the URLOperation capabilities.

Update changelog
This addresses the iOS tests that are leaking pings to the ingestion pipeline and are showing up in the Glean Debug View. This wraps any call to `resetGlean` with an OHTTPStub in order to intercept the request and prevent the network activity. Most test cases just reset Glean in the `setUp` function, and so the startup pings are now intercepted so they don't interfere with the tests or escape. In some cases, where we were already using stubs as we test contents of pings, the intent of the test was maintained and any setup calls to `resetGlean` were also wrapped in a way to not interfere with these tests' intents.
@travis79 travis79 force-pushed the playing-with-ios-fire branch from ed450c5 to 65dfe08 Compare September 28, 2021 21:21
@travis79 travis79 requested a review from badboy September 28, 2021 21:49
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Some small things left, but with those changed we can land it.

CHANGELOG.md Outdated Show resolved Hide resolved
glean-core/ios/Glean/Net/HttpPingUploader.swift Outdated Show resolved Hide resolved
Glean.shared.setUploadEnabled(true)
OHHTTPStubs.removeAllStubs()
expectation = nil
tearDownStubs()
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

🧱 🇩🇪

glean-core/ios/GleanTests/TestUtils.swift Outdated Show resolved Hide resolved
@travis79 travis79 merged commit a0dd8e5 into mozilla:main Sep 29, 2021
@travis79 travis79 deleted the playing-with-ios-fire branch September 29, 2021 13:08
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