Skip to content

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented May 18, 2020

Add a FakeConsole test class along with a URLSession subclass for running Remote Config tests without networking and enabling console changes while testing.

Also delete the no-longer used globalTestBlock that was a more complicated fake for faking the fetch closer to the network call.

settings.minimumFetchInterval = 0
config.configSettings = settings

FakeFetch.config = ["Key1": "Value1"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Initialize the fake console.

@@ -0,0 +1,28 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member Author

Choose a reason for hiding this comment

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

Dummy GoogleService-Info.plist

// See the License for the specific language governing permissions and
// limitations under the License.

#import <FirebaseRemoteConfig/RCNFakeFetch.h>
Copy link
Member Author

Choose a reason for hiding this comment

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

Makes the RCNFakeFetch visible to Swift tests.

+ (void)setGlobalTestBlock:(RCNConfigFetcherTestBlock)block {
FIRLogDebug(kFIRLoggerRemoteConfig, @"I-RCN000027",
@"Set global test block for NSSessionFetcher, it will not fetch from server.");
gGlobalTestBlock = [block copy];
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like using a test block is simpler and doesn't take a lot of test code in production. And you can customize the block in test rather than having all the test logic in production code. You can also provide flexible return value (not just success) to test different scenarios.

This is used in a lot of the unit tests of remote config (search setGlobalTestBlock in tests), but I'm not sure the current state of how many of those tests are running but if we remove this, we might also have to adjust all those unit test as well.

Another thing is do we want to fake fetch in integration test since in github network is available. isn't it better to actually enable fetch from prod server?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test block is only used in disabled unit tests that still need to be ported from RC v1 to v2. This implementation allows the tests to be implemented without any knowledge of the library implementation details - just a simple API of managing a dictionary.

The swift_api tests are similar to these but do fetches from a prod server. This set of tests are to enable API tests that require midstream console changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

As for me, for unit tests I would prefer injecting another implementation of RCNConfigFetch for testing purposes instead of adding the test code to production with either the test block or RCNFakeFetch. For this we ideally should replace dependency on RCNConfigFetch to a protocol dependency like RCNConfigFetchProtocol or use a mock implementation for tests like here (less preferred). Anyway, we should allow passing RCNConfigFetch as an init parameter for tests, then we can avoid the test code in production but keep the code testable.

For integration end-to-end tests, in contrast to unit tests, I would suggest not to mock anything and use real implementations only.

+ (void)setGlobalTestBlock:(RCNConfigFetcherTestBlock)block {
FIRLogDebug(kFIRLoggerRemoteConfig, @"I-RCN000027",
@"Set global test block for NSSessionFetcher, it will not fetch from server.");
gGlobalTestBlock = [block copy];
Copy link
Contributor

Choose a reason for hiding this comment

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

As for me, for unit tests I would prefer injecting another implementation of RCNConfigFetch for testing purposes instead of adding the test code to production with either the test block or RCNFakeFetch. For this we ideally should replace dependency on RCNConfigFetch to a protocol dependency like RCNConfigFetchProtocol or use a mock implementation for tests like here (less preferred). Anyway, we should allow passing RCNConfigFetch as an init parameter for tests, then we can avoid the test code in production but keep the code testable.

For integration end-to-end tests, in contrast to unit tests, I would suggest not to mock anything and use real implementations only.


import XCTest

class APITests: XCTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure we are on the same page: are these tests supposed to test, RCNRemoteConfig class implementation or integration of all remote config components? If the later, we should not mock remote config classes but rather Foundation classes, otherwise we would not tests the actual code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@maksymmalyhin These are intended to be as close as possible to integration tests while enabling changes to the console which is not possible in pure integration tests and a source of several bugs. The point of RCNFakeFetch is to enable a dictionary API for tests to simulate console changes.

Copy link
Member Author

Choose a reason for hiding this comment

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