Moving the StreamTest to integration tests #391

schmidt-sebastian · 2017-10-19T17:14:04Z

This is meant to make #388 smaller.

wilhuff · 2017-10-20T22:57:07Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

+  _expectation = nil;
+}
+
+- (void)fulfillOnCallback:(XCTestExpectation *)expectation {


This name is off: the trailing part of the label should be "expectation" to match the argument on the other side of the type.

Meanwhile every usage of this

declares the expectation

calls _delegate fulfillOnCallback

does some async thing

calls await expectations

Why not write a routine that takes a block and wraps it with the first two steps then calls the block then finally awaits expectations.

Something like:

- (void)awaitCallbackInBlock:(blocktype)block { XCTestExpectation *openExpectation = [_testCase expectationWithDescription:@"awaiting"]; [_delegate fulfillOnCallback:openExpectation]; block(); [_testCase awaitExpectations]; }

Sounds good, done.

wilhuff · 2017-10-20T23:13:13Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

+  return [datastore createWatchStreamWithDelegate:_delegate];
+}
+
+- (void)verifyDelegate:(NSArray<NSString *> *)expectedStates {


Naming: label should end in states. Perhaps verifyDelegateObservedStates?

When you're doing objective-C naming right each argument should be repeated two or three times: the label, type, and argument should align.

Sometimes the type differs because the label/argument are more specific than the type (as would be in this case).

Done, thanks for the explanation.

wilhuff · 2017-10-20T23:13:53Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

+}
+
+- (void)tearDown {
+  [super tearDown];


You can omit this method if it's not doing anything.

wilhuff · 2017-10-22T19:21:12Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

+  dispatch_sync(_testQueue, ^{
+                });
+
+  XCTAssertEqualObjects(_delegate.states, expectedStates);


Shouldn't this also remove the states accumulated in the delegate so that subsequent assertions only need to assert about the new states?

Hm, we can do this here, but it's not technically needed since delegate is scoped to the test.

delegate is scoped to the test but not clearing this means that you can't reasonably make more than one assertion. I'm also fine just leaving this alone until we need to do that, but just wanted the intent of my comment to be clear.

schmidt-sebastian · 2017-10-23T18:31:06Z

Feedback addressed.

wilhuff · 2017-10-23T18:47:43Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

@@ -38,17 +38,25 @@ - (void)writesFinishedWithError:(NSError *_Nullable)error;
 */
 @interface FSTStreamStatusDelegate : NSObject <FSTWatchStreamDelegate, FSTWriteStreamDelegate>

+- (instancetype)initFrom:(XCTestCase *)testCase


Naming: first part of the label should end with "test case".

Also avoid connecting words in the labels for subsequent arguments unless they're critical to the meaning. It's appropriate in e.g. copyToFile:fromFile but not here, where you're just specifying the queue argument.

This should be: initWithTestCase:(XCTestCase *)testCase queue:(FSTDispatchQueue *)queue.

This was inspired by the name in FEventTester. I updated it to use the naming you suggested and removed the 'using' part.

wilhuff · 2017-10-23T18:52:10Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

 @property(nonatomic, readonly) NSMutableArray<NSString *> *states;
 @property(atomic, readwrite) BOOL invokeCallbacks;
 @property(nonatomic, weak) XCTestExpectation *expectation;
 @property(nonatomic, weak, readonly) FSTStream *stream;
+@property(nonatomic, weak, readonly) XCTestCase *testCase;
+@property(nonatomic, weak, readonly) FSTDispatchQueue *dispatchQueue;


There are a lot of weak references in here which is a serious design smell. Are these actually necessary?

In general avoid weak references unless you specifically need them to break a reference cycle because they make your code very difficult to reason about.

In this case, you need the testCase to be weak because you're making this a member of that test case (which creates a cycle). The other things in here can be regular strong references.

It's enough for 'testCase' to be weak. Everything else is only retained by the test case itself as well as 'FSTStreamStatusDelegate'.

wilhuff · 2017-10-23T18:54:57Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

+- (instancetype)initFrom:(XCTestCase *)testCase
+              usingQueue:(FSTDispatchQueue *)dispatchQueue NS_DESIGNATED_INITIALIZER;
+- (instancetype)init NS_UNAVAILABLE;
+
 @property(nonatomic, readonly) NSMutableArray<NSString *> *states;


Consider splitting out implementation properties into a separate interface with an empty category (as is our convention elsewhere). As it stands it's not clear which of these are implementation state and which are meant to be used from within tests that use this delegate.

The only public-facing property was 'invokeCallbacks', but I am actually not using it in this PR (and have removed it from the other one as well).

I tried moving the other properties into their own category, but it looks like I then need to create my getters and setters by hand (even @synthesize doesn't work). There's some discussion about this here: https://stackoverflow.com/questions/14399991/how-can-i-declare-a-private-property-within-a-named-category

wilhuff · 2017-10-23T18:55:40Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

 @property(nonatomic, readonly) NSMutableArray<NSString *> *states;
 @property(atomic, readwrite) BOOL invokeCallbacks;
 @property(nonatomic, weak) XCTestExpectation *expectation;
 @property(nonatomic, weak, readonly) FSTStream *stream;
+@property(nonatomic, weak, readonly) XCTestCase *testCase;


nit: consider making your declaration order match your assignment order in the constructor, so it's easy to verify everything is assigned.

wilhuff · 2017-10-23T18:57:30Z

Firestore/Example/Tests/Integration/FSTStreamTests.m

+  dispatch_sync(_testQueue, ^{
+                });
+
+  XCTAssertEqualObjects(_delegate.states, expectedStates);


delegate is scoped to the test but not clearing this means that you can't reasonably make more than one assertion. I'm also fine just leaving this alone until we need to do that, but just wanted the intent of my comment to be clear.

wilhuff

This turned out really nicely. Thanks for pushing through all these changes.

Please verify the travis failure isn't real before submitting.

Closing the write and watch stream after 60s of idleness

7280673

googlebot added the cla: yes label Oct 19, 2017

Verifying the write handshake in FSTStreamTest

958f866

schmidt-sebastian force-pushed the mrschmidt-streamtests branch from 87932ca to 958f866 Compare October 19, 2017 17:18

schmidt-sebastian assigned wilhuff Oct 19, 2017

schmidt-sebastian mentioned this pull request Oct 20, 2017

Closing the write and watch stream after 60s of idleness #388

Merged

wilhuff reviewed Oct 22, 2017

View reviewed changes

Cleaning up test

096e77c

schmidt-sebastian force-pushed the mrschmidt-streamtests branch from 974162d to 096e77c Compare October 23, 2017 18:29

schmidt-sebastian closed this Oct 23, 2017

schmidt-sebastian reopened this Oct 23, 2017

wilhuff reviewed Oct 23, 2017

View reviewed changes

Cleaning up properties and naming

901b4c8

wilhuff approved these changes Oct 24, 2017

View reviewed changes

schmidt-sebastian merged commit 8bdce1d into master Oct 24, 2017

schmidt-sebastian mentioned this pull request Oct 24, 2017

Creating Stream Tests for the Web Client firebase/firebase-js-sdk#260

Merged

schmidt-sebastian deleted the mrschmidt-streamtests branch October 31, 2017 01:21

minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018

Moving the StreamTest to integration tests (firebase#391)

fedc024

firebase locked and limited conversation to collaborators Nov 12, 2019

Moving the StreamTest to integration tests #391

Moving the StreamTest to integration tests #391

Uh oh!

Conversation

schmidt-sebastian commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian Oct 23, 2017 •

edited

Loading