-
Notifications
You must be signed in to change notification settings - Fork 168
[RUM-16386] Add RemoteConfigurationProvider, typed errors, and computed var remoteConfiguration #2980
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
base: feature/remote-config
Are you sure you want to change the base?
[RUM-16386] Add RemoteConfigurationProvider, typed errors, and computed var remoteConfiguration #2980
Changes from 1 commit
4dad04f
f481452
d70251b
405b326
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,22 @@ private func mockSession() -> URLSession { | |
| return URLSession(configuration: config) | ||
| } | ||
|
|
||
| private final class FetchHTTPClientMock: HTTPClient { | ||
| let result: Result<(HTTPURLResponse, Data), Error> | ||
|
|
||
| init(result: Result<(HTTPURLResponse, Data), Error>) { | ||
| self.result = result | ||
| } | ||
|
|
||
| func send(request: URLRequest, delegate: URLSessionTaskDelegate?, completion: @escaping (Result<HTTPURLResponse, Error>) -> Void) { | ||
| completion(.failure(URLSessionTransportInconsistencyException())) | ||
| } | ||
|
|
||
| func fetch(request: URLRequest, completion: @escaping (Result<(HTTPURLResponse, Data), Error>) -> Void) { | ||
| completion(result) | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed this in previous PR, but I think we should rely on a single func send(request: URLRequest, delegate: URLSessionTaskDelegate?, completion: @escaping (Result<(HTTPURLResponse, Data?), Error>) -> VoidThis clarify the usage and avoid having to provide a default implementation in the protocol extension. |
||
|
|
||
| // MARK: - Tests | ||
|
|
||
| class RemoteConfigurationTests: XCTestCase { | ||
|
|
@@ -164,14 +180,20 @@ class RemoteConfigurationTests: XCTestCase { | |
| } | ||
|
|
||
| func testSyncNetworkErrorReturnsFailureAndLeavesCache() { | ||
| MockURLProtocol.requestHandler = { _ in throw URLError(.networkConnectionLost) } | ||
| let rc = makeSynchronizer() | ||
| let error = URLError(.networkConnectionLost) | ||
| let rc = RemoteConfigurationSynchronizer( | ||
| id: "test-id", | ||
| site: .us1, | ||
| directory: coreDir.coreDirectory, | ||
| httpClient: FetchHTTPClientMock(result: .failure(error)) | ||
| ) | ||
| let expectation = expectation(description: "sync completes") | ||
|
|
||
| rc.sync { result in | ||
| guard case .failure = result else { | ||
| guard case .failure(.networkError(let receivedError as URLError)) = result else { | ||
| return XCTFail("Expected failure on network error") | ||
| } | ||
| XCTAssertEqual(receivedError.code, error.code) | ||
| expectation.fulfill() | ||
| } | ||
| wait(for: [expectation], timeout: 2) | ||
|
|
@@ -238,6 +260,9 @@ class RemoteConfigurationTests: XCTestCase { | |
| expectation.fulfill() | ||
| } | ||
| wait(for: [expectation], timeout: 2) | ||
|
|
||
| XCTAssertEqual(try? rc.cache.get(), nonJSON) | ||
| XCTAssertEqual(try? Data(contentsOf: coreDir.coreDirectory.url.appendingPathComponent("test-id.json")), nonJSON) | ||
| } | ||
|
|
||
| func testSyncDiskWriteFailureReturnsFailure() { | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This protocol belongs to the |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||
| /* | ||||||
| * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. | ||||||
| * This product includes software developed at Datadog (https://www.datadoghq.com/). | ||||||
| * Copyright 2019-Present Datadog, Inc. | ||||||
| */ | ||||||
|
|
||||||
| import Foundation | ||||||
|
|
||||||
| /// Provides the last successfully fetched remote configuration. | ||||||
| public protocol RemoteConfigurationProvider { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This adds a new Useful? React with 👍 / 👎. |
||||||
| /// The last successfully fetched and decoded remote configuration, if any. | ||||||
| var remoteConfiguration: RemoteConfiguration? { get } | ||||||
| } | ||||||
|
|
||||||
| /// Default implementation that returns no remote configuration. | ||||||
| public struct DefaultRemoteConfigurationProvider: RemoteConfigurationProvider { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| public init() { } | ||||||
|
|
||||||
| public var remoteConfiguration: RemoteConfiguration? { nil } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove the
RemoteConfigurationSynchronizer:)