Fake Remote Config console for testing #5633

paulb777 · 2020-05-18T18:34:36Z

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.

FirebaseRemoteConfig/Tests/HermeticAPI/APITests.swift

paulb777 · 2020-05-18T19:47:54Z

FirebaseRemoteConfig/Tests/HermeticAPI/APITests.swift

+    settings.minimumFetchInterval = 0
+    config.configSettings = settings
+
+    FakeFetch.config = ["Key1": "Value1"]


Initialize the fake console.

paulb777 · 2020-05-18T19:48:14Z

FirebaseRemoteConfig/Tests/HermeticAPI/GoogleService-Info.plist

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


Dummy GoogleService-Info.plist

paulb777 · 2020-05-18T19:48:31Z

FirebaseRemoteConfig/Tests/HermeticAPI/Bridging-Header.h

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#import <FirebaseRemoteConfig/RCNFakeFetch.h>


Makes the RCNFakeFetch visible to Swift tests.

FirebaseRemoteConfig/Tests/HermeticAPI/APITests.swift

charlotteliang · 2020-05-18T20:04:33Z

FirebaseRemoteConfig/Sources/RCNFetch.m

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


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?

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.

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.

FirebaseRemoteConfig/Sources/RCNFetch.m

maksymmalyhin · 2020-05-19T16:58:09Z

FirebaseRemoteConfig/Sources/RCNFetch.m

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


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.

maksymmalyhin · 2020-05-19T17:04:33Z

FirebaseRemoteConfig/Tests/HermeticAPI/APITests.swift

+
+import XCTest
+
+class APITests: XCTestCase {


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.

@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.

@maksymmalyhin I went with the approach of partially mocking URLSession. This seems to be a less-intrusive approach than injecting another RCNConfigFetch. Besides changing URLSession, it required a flag and two early exits from two RCNConfigFetch methods doing unrelated networking. PTAL

ncooke3

This is a cool way to do this. It makes sense to me and doesn’t seem to leave much testing code in production. 👍

paulb777 · 2020-05-22T22:18:24Z

.travis.yml

@@ -194,7 +194,7 @@ jobs:
      env:
        - PROJECT=RemoteConfig METHOD=pod-lib-lint
      script:
-        - travis_retry ./scripts/if_changed.sh ./scripts/pod_lib_lint.rb FirebaseRemoteConfig.podspec --platforms=ios
+        - travis_retry ./scripts/if_changed.sh ./scripts/pod_lib_lint.rb FirebaseRemoteConfig.podspec --platforms=ios --skip-tests


pod lib lint tests don't work now that there's multiple RC test_specs. All tests are run in GHA.

maksymmalyhin

Thanks Paul, that looks much better to me! It still seams we can avoid having a tests specific code in production (see comments).

maksymmalyhin · 2020-05-26T14:46:51Z

FirebaseRemoteConfig/Sources/RCNFetch.m

@@ -204,6 +205,10 @@ - (NSString *)FIRAppNameFromFullyQualifiedNamespace {
 /// requests to work.(b/14751422).
 - (void)refreshInstallationsTokenWithCompletionHandler:
    (FIRRemoteConfigFetchCompletion)completionHandler {
+  if (self.testWithoutNetwork) {


I wonder what makes this flag necessary? Would a test utils with mock dependencies like in FCM solve the issue without changes to the production code?

Eliminated the testWithoutNetwork flag by mocking refreshInstallationsTokenWithCompletionHandler:. Mocking the dependencies seems more complicated and may be better to do in conjunction with a bigger refactor of RCNFetch.

maksymmalyhin · 2020-05-26T14:51:53Z

FirebaseRemoteConfig/Sources/RCNFetch.m

@@ -111,6 +109,9 @@ - (instancetype)initWithContent:(RCNConfigContent *)content

 /// Force a new NSURLSession creation for updated config.
 - (void)recreateNetworkSession {
+  if (self.testWithoutNetwork) {


What if we inject implementation of newFetchSession method (e.g. with a block) as instead? Or we can use OCMock to stub + [NSURLSession sessionWithConfiguration:] method to avoid the class changes altogether.

This one is also tricky, since we still need the first newFetchSession to happen before any mocking. Thus, I went with the more expedient mock of recreateNetworkSession until we do a bigger refactor.

maksymmalyhin

LGTM. I still can see room for improvements 😛, but nothing blocking from my point of view.

maksymmalyhin · 2020-05-26T19:18:45Z

FirebaseRemoteConfig/Tests/SwiftAPI/APITestBase.swift

+      config.configFetch.fetchSession = URLSessionMock(with: fakeConsole)
+      if !APITests.mockedFetch {
+        APITests.mockedFetch = true
+        config.configFetch = FetchMocks.mockFetch(config.configFetch)


Looks fine if we accept that RCNConfigFetch is not properly verified by the test.

maksymmalyhin · 2020-05-26T19:23:53Z

FirebaseRemoteConfig/Tests/FakeUtils/FetchMocks.m

+  RCNConfigFetch *mock = OCMPartialMock(fetch);
+  OCMStub([mock recreateNetworkSession]).andDo(nil);
+  OCMStub([mock refreshInstallationsTokenWithCompletionHandler:[OCMArg any]])
+      .andCall(mock, @selector(doFetchCall:));


Reliance on private methods can make refactoring a bit more painful in the future.

charlotteliang · 2020-05-26T21:02:17Z

FirebaseRemoteConfig/Sources/Private/RCNConfigFetch.h

@@ -56,8 +53,8 @@ typedef void (^RCNConfigFetcherTestBlock)(RCNConfigFetcherCompletion completion)
 /// Add the ability to update NSURLSession's timeout after a session has already been created.
 - (void)recreateNetworkSession;

-/// Sets the test block to mock the fetch response instead of performing the fetch task from server.
-+ (void)setGlobalTestBlock:(RCNConfigFetcherTestBlock)block;
+/// Provide fetchSession for tests to override.


I like this better!

karenyz · 2020-05-26T21:56:18Z

FirebaseRemoteConfig/Tests/FakeConsole/FakeConsoleTests.swift

+      XCTAssertEqual(status, RemoteConfigFetchStatus.success)
+      self.config.activate { error in
+        if let error = error {
+          print("Activate Error \(error)")


I'm guessing this should also be removed :)

With the deprecated API whether or not there is an error on the first activate call is random depending on the state of the console - so it might be useful to keep until we can delete along with the deprecated API at the next major release.

paulb777 · 2020-05-26T23:14:16Z

I'm going to merge now to start lining up for M72. If there are more comments, I'll address in a subsequent PR.

googlebot added the cla: yes label May 18, 2020

google-oss-bot added the api: remoteconfig label May 18, 2020

paulb777 commented May 18, 2020

View reviewed changes

paulb777 requested review from charlotteliang and karenyz May 18, 2020 19:55

charlotteliang reviewed May 18, 2020

View reviewed changes

paulb777 requested review from ryanwilson and maksymmalyhin May 19, 2020 16:36

maksymmalyhin reviewed May 19, 2020

View reviewed changes

paulb777 mentioned this pull request May 19, 2020

Better completion callback for activate #5646

Merged

paulb777 added 9 commits May 21, 2020 13:59

Initial Fake Remote Config backend

a4c1b7d

yml and cleanup

04be952

style and changelog

0809e79

Fix analyze

5e188fa

Remove unused global for old serverless testing

5dd5182

wip - Mock console

c087d43

initial mock

c7f7b8a

checkpoint

01f2cb0

cleanup

2441967

paulb777 force-pushed the pb-rc-fake branch from 6ca8c81 to 2441967 Compare May 22, 2020 00:18

paulb777 requested a review from ncooke3 May 22, 2020 00:18

paulb777 added 2 commits May 22, 2020 08:22

FakeConsole does not need to be a singleton

9039bee

cleanup

db529b1

ncooke3 reviewed May 22, 2020

View reviewed changes

skip pll tests on travis now that secrets are needed

79b0e41

paulb777 commented May 22, 2020

View reviewed changes

paulb777 changed the title ~~Initial Fake Remote Config backend~~ May 22, 2020

paulb777 added 2 commits May 24, 2020 10:51

DRY up the test organization

9745af3

fix names

8b078f4

maksymmalyhin reviewed May 26, 2020

View reviewed changes

paulb777 added 2 commits May 26, 2020 11:58

Use OCMock to eliminate testWithoutNetwork change in library

190030c

style

98d3dab

maksymmalyhin approved these changes May 26, 2020

View reviewed changes

charlotteliang approved these changes May 26, 2020

View reviewed changes

karenyz reviewed May 26, 2020

View reviewed changes

paulb777 merged commit cc233d6 into master May 26, 2020

paulb777 deleted the pb-rc-fake branch May 26, 2020 23:14

firebase locked and limited conversation to collaborators Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fake Remote Config console for testing #5633

Fake Remote Config console for testing #5633

paulb777 commented May 18, 2020 •

edited

Loading

paulb777 May 18, 2020

paulb777 May 18, 2020

paulb777 May 18, 2020

charlotteliang May 18, 2020

paulb777 May 18, 2020

maksymmalyhin May 19, 2020

maksymmalyhin May 19, 2020

maksymmalyhin May 19, 2020

paulb777 May 19, 2020

paulb777 May 26, 2020

ncooke3 left a comment

paulb777 May 22, 2020

maksymmalyhin left a comment

maksymmalyhin May 26, 2020

paulb777 May 26, 2020

maksymmalyhin May 26, 2020

paulb777 May 26, 2020

maksymmalyhin left a comment

maksymmalyhin May 26, 2020

maksymmalyhin May 26, 2020

charlotteliang May 26, 2020

karenyz May 26, 2020

paulb777 May 26, 2020

paulb777 commented May 26, 2020

Fake Remote Config console for testing #5633

Fake Remote Config console for testing #5633

Conversation

paulb777 commented May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncooke3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksymmalyhin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksymmalyhin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulb777 commented May 26, 2020

paulb777 commented May 18, 2020 •

edited

Loading