Bugfix/metrics dispatcher cicd #131
Conversation
Bump MARKETING_VERSION
Add FASTLANE_XCODEBUILD_SETTINGS_TIMEOUT to ios platform
Set Scan clean to false
call clearSummedEvents before metrics tests
melekr
force-pushed
the
bugfix/metrics-dispatcher-cicd
branch
from
7db5967 to
e5f2ab2
Compare
melekr
force-pushed
the
bugfix/metrics-dispatcher-cicd
branch
from
e5f2ab2 to
4f57ead
Compare
| "location.enabled": CLLocationManager.locationServicesEnabled(), | ||
| "location.authorization.status": CLLocationManager.authorizationStatus().name | ||
| ] | ||
| var mutableDict = [String: Any?]() |
There was a problem hiding this comment.
I think it won't work. The location information won't be returned on line 214 but will be generated later.
There was a problem hiding this comment.
seemed to work when I tested, I'll double check.
There was a problem hiding this comment.
Maybe we should remove it because it doesn't make sense to keep it and this is not something our customers want to use. If they want (their clients need to agree to use location services) then can pass them to use via attribuets
Tests/BacktraceMetricsTest.swift
Outdated
|
|
||
| beforeEach { | ||
| metrics = BacktraceMetrics(api: backtraceApi) | ||
| metrics?.enable(settings: BacktraceMetricsSettings()) |
There was a problem hiding this comment.
enable method should generate a summed + unique event. Are you sure we're testing the right thing? Maybe we should clear summed events if we want to test unique? I think its worth testing if the default summed +unique events are generated.
There was a problem hiding this comment.
all I did was cleanup by moving metrics init and enable to beforeEach.
Tests premises remain unchanged.
I added clearSummedEvents to test but it's not needed at all. I'll revert tests to the way they were.
There was a problem hiding this comment.
I think we're not testing the most important functionality, which is checking if the unique/summed event will be generated on the app startup. Maybe we can move cleanup method to each test and allow the test to decide what is the behavior under the test?
Tests/BacktraceMetricsTest.swift
Outdated
|
|
||
| it("clears the summed event after enabling and sending") { | ||
| // Allow default events to be "sent" out | ||
| expect { metrics?.count }.toEventually(equal(1)) |
There was a problem hiding this comment.
why 1? I think its becuase the unique event is still there. Maybe we can do better and get all events and filter them by checking the event type? Maybe it is just enough if we move 1 to variable and call it expectedNumberOfEvents
There was a problem hiding this comment.
I think we should verify it the number of summed events is 1. metrics.count will return number of events in the queue (unique + summed)
Tests/BacktraceMetricsTest.swift
Outdated
|
|
||
| beforeEach { | ||
| metrics = BacktraceMetrics(api: backtraceApi) | ||
| metrics?.enable(settings: BacktraceMetricsSettings()) |
There was a problem hiding this comment.
I think we're not testing the most important functionality, which is checking if the unique/summed event will be generated on the app startup. Maybe we can move cleanup method to each test and allow the test to decide what is the behavior under the test?
Tests/BacktraceMetricsTest.swift
Outdated
|
|
||
| it("clears the summed event after enabling and sending") { | ||
| // Allow default events to be "sent" out | ||
| expect { metrics?.count }.toEventually(equal(1)) |
There was a problem hiding this comment.
I think we should verify it the number of summed events is 1. metrics.count will return number of events in the queue (unique + summed)
Tests/BacktraceMetricsTest.swift
Outdated
| metrics?.addSummedEvent(name: summedEventName) | ||
| expect { metrics?.count }.to(equal(2)) |
There was a problem hiding this comment.
I think the test is about to verify if the number of tests added to the queue will increase. I think that's the better way to verify that. We shouldn't think about hard coded numbers but rather about the behavior
| metrics?.addSummedEvent(name: summedEventName) | |
| expect { metrics?.count }.to(equal(2)) | |
| let count = metrics?.count | |
| metrics?.addSummedEvent(name: summedEventName) | |
| expect { metrics?.count }.to(equal(count + 1)) |
Also maybe it would be better to verify if the summedEventName is in the metrics list?
Add getSummedEvents and getUniqueEvents functions Update BacktraceMetricsTests
Update swift package platforms versions
This diff contains the following updates:
ref: BT-3005