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 1586738 - Add Glean-iOS MetricsPingScheduler #655

Merged
merged 4 commits into from
Jan 22, 2020

Conversation

travis79
Copy link
Member

This adds the MetricsPingScheduler to the iOS bindings.

@auto-assign auto-assign bot requested a review from badboy January 15, 2020 19:37
@codecov-io
Copy link

codecov-io commented Jan 15, 2020

Codecov Report

Merging #655 into master will increase coverage by 0.59%.
The diff coverage is 98.79%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #655      +/-   ##
============================================
+ Coverage     74.69%   75.28%   +0.59%     
  Complexity      266      266              
============================================
  Files            96       97       +1     
  Lines          5936     6070     +134     
  Branches        730      730              
============================================
+ Hits           4434     4570     +136     
+ Misses          966      964       -2     
  Partials        536      536
Impacted Files Coverage Δ Complexity Δ
glean-core/ios/Glean/Utils/Utils.swift 94.38% <100%> (-0.25%) 0 <0> (ø)
...ore/ios/Glean/Scheduler/MetricsPingScheduler.swift 100% <100%> (ø) 0 <0> (?)
glean-core/ios/Glean/Glean.swift 92.65% <90.9%> (+1.12%) 0 <0> (ø) ⬇️

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 46ec798...8ea3514. 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.

I need to give this another look, especially the tests.
For a first pass I found a few nitpicky things, but overall this looks like some very solid work already.

@travis79
Copy link
Member Author

I need to give this another look, especially the tests.
For a first pass I found a few nitpicky things, but overall this looks like some very solid work already.

Please do check it carefully, the last two tests in the file were especially troublesome due to async hell.

@badboy badboy self-requested a review January 17, 2020 12:25
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.

Do we have any way to modify the clock used in the test to e.g. simulate the time hopping a bit better?
That way we could also test timezones changes more reliably.

@travis79
Copy link
Member Author

Do we have any way to modify the clock used in the test to e.g. simulate the time hopping a bit better?
That way we could also test timezones changes more reliably.

We can change timezones but setting the date/time, even for tests seems to be possible only by scripting it from CLI. This SO post was the best I found from a quick search. Go Apple!

@Dexterp37
Copy link
Contributor

Given @mdboom experience with the Android implementation, I'd recommend he takes at least one high level look at this.

@badboy badboy requested a review from mdboom January 20, 2020 09:12
Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

LGTM. Ideally, let's merge this before #649, and then have #649 add the reason codes for iOS.


/// Overloads the operator so that subtraction between two dates results in a TimeInterval representing
/// the difference between them
static func - (lhs: Date, rhs: Date) -> TimeInterval {
Copy link
Member

Choose a reason for hiding this comment

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

static func - ... it does look weird.

@travis79 travis79 merged commit 19af88c into mozilla:master Jan 22, 2020
@travis79 travis79 deleted the Bug1586738 branch January 22, 2020 17:15
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.

5 participants