-places tests #3
Conversation
AEPPlaces/Sources/Places.swift
Outdated
| @@ -101,15 +113,17 @@ public class Places: NSObject, Extension { | |||
|
|
|||
| // MARK: - Private Methods | |||
| private func processSharedStateChange(event: Event) { | |||
There was a problem hiding this comment.
For better clarity can we rename this method from processSharedStateChange to processConfigStateChange
Since handleSharedStateUpdate filters out all the other shared state changes
AEPPlaces/Tests/PlacesTests.swift
Outdated
| // onRegistered called in setUpWithError | ||
|
|
||
| // test | ||
| mockRuntime.startEvents() |
There was a problem hiding this comment.
any significance in calling startEvents ?
| func testClear() throws { | ||
| // setup | ||
| let expectation = XCTestExpectation(description: "clear should dispatch an event") | ||
| expectation.assertForOverFulfill = true |
There was a problem hiding this comment.
If I am not wrong I don't think it's necessary to explicitly mention
expectation.assertForOverFulfill = true
It's true by default. If so we can remove setting that property in all the tests
| // MARK: - getCurrentPointsOfInterest | ||
| func testGetCurrentPointsOfInterest() throws { | ||
| // setup | ||
| let expectation = XCTestExpectation(description: "getCurrentPointsOfInterest should call its closure") |
There was a problem hiding this comment.
Isn't the expectation of the test is to see if the API call dispatches the event?
In that case, can we rename the description to
getCurrentPointsOfInterest should dispatch places request content event
There was a problem hiding this comment.
yep, you're right on this
| expectation.assertForOverFulfill = true | ||
| extensionContainer.registerListener(type: EventType.places, source: EventSource.requestContent) { (event) in | ||
| XCTAssertEqual(PlacesConstants.EventDataKey.Places.RequestType.GET_NEARBY_PLACES, | ||
| event.data?[PlacesConstants.EventDataKey.Places.REQUEST_TYPE] as? String) |
There was a problem hiding this comment.
Can we assert with eventData on
"count" : 3,
"longitude" : 23.45
"latitude" : 12.34,
??
| expectation.assertForOverFulfill = true | ||
| extensionContainer.registerListener(type: EventType.places, source: EventSource.requestContent) { (event) in | ||
| XCTAssertEqual(PlacesConstants.EventDataKey.Places.RequestType.PROCESS_REGION_EVENT, | ||
| event.data?[PlacesConstants.EventDataKey.Places.REQUEST_TYPE] as? String) |
There was a problem hiding this comment.
can we assert on eventData for entrytype and regionID
"regioneventtype" : "entry",
"requesttype" : "requestprocessregionevent",
"regionid" : "id"
And similar kind of assertions on eventData for other API tests in this class
AEPPlaces/Tests/PlacesTests.swift
Outdated
| // verify | ||
| XCTAssertNil(mockQueryService.invokedLat) | ||
| XCTAssertNil(mockQueryService.invokedLon) | ||
| XCTAssertNil(mockQueryService.invokedCount) |
There was a problem hiding this comment.
Nit : Instead of checking for nil on Lat, Lon, and count
A better way is to have a boolean isInvoked in mockQueryService that gets set when getNearByPOI method is called
And these 3 check can be replaced by just
XCTAssertFalse(mockQueryService.isInvoked)
This will to few more test cases below
AEPPlaces/Tests/PlacesTests.swift
Outdated
| XCTAssertEqual(EventSource.responseContent, responseEvent.source) | ||
| XCTAssertEqual(PlacesConstants.EventName.Response.GET_NEARBY_PLACES, responseEvent.name) | ||
| let dispatchedData = responseEvent.data! | ||
| XCTAssertEqual(0, dispatchedData.count) |
There was a problem hiding this comment.
In Android when privacy status is opted out... eventData contains "status" key with privacyoptOut response error
Can we add them here as well and not dispatch a empty eventData?
| XCTAssertEqual(0, mockRuntime.dispatchedEvents.count) | ||
| } | ||
|
|
||
| func testHandleProcessRegionEventRequestPlacesConfigMissing() throws { |
There was a problem hiding this comment.
technically handling a region entry/exit event doesn't need places configuration.
I like the fact that when no configuration we stop processing all the events. If this is correct, may be we should introduce the same changes to Android
There was a problem hiding this comment.
we don't allow entry unless the region is in nearbyPois, and nearbyPois can't get populated without a proper places configuration, so i guess technically it does.
i think we could discuss if this is proper behavior though :)
No description provided.