Push Channels tests #708

ricardopereira · 2018-03-23T15:27:34Z

No description provided.

funkyboy

Some comments and requested changes.

funkyboy · 2018-04-03T14:54:11Z

Source/ARTPushChannel.m

- (void)listSubscriptions:(void(^)(ARTPaginatedResult<ARTPushChannelSubscription *> *_Nullable, ARTErrorInfo *_Nullable))callback {
-    [self listSubscriptions:nil callback:callback];
-}
+- (BOOL)listSubscriptions:(NSDictionary<NSString *, NSString *> *)params callback:(void(^)(ARTPaginatedResult<ARTPushChannelSubscription *> *_Nullable, ARTErrorInfo *_Nullable))callback error:(NSError *_Nullable *_Nullable)errorPtr {


I don't understand why we need to return a boolean.
We don't test it and the callback already handles both paginated results and potential errors.

If there isn't a good reason I'd go back to a method that returns void.

It has a BOOL because it's how ObjC handle errors. We should respect that for good Swift/ObjC interoperability. An ObjC method with an NSError ** as last parameter and a BOOL return type will bridge to a Swift method that throw an error.

funkyboy · 2018-04-03T14:57:50Z

Source/ARTPushChannel.m

-    if (!device) {
+- (ARTLocalDevice *)getDevice:(void(^_Nullable)(ARTErrorInfo *_Nullable))callback {
+    ARTLocalDevice *device = [_channel device];
+    if (![device isRegistered]) {
        if (callback) callback([ARTErrorInfo createWithCode:0 message:@"cannot use device before ARTRest.push.activate has finished"]);


Not sure we should mention "ARTRest.push.activate".
I think "... before device activation has finished" is better.

Accepted acd4100 👍

funkyboy · 2018-04-03T15:04:52Z

Spec/PushChannel.swift

+                    }
+                }
+
+                // RSH7a2


I think this is RSH7b2

Fixed 2dba0b0 👍

funkyboy · 2018-04-03T15:16:55Z

Spec/PushChannel.swift

+
+                it("should return a paginated result with PushChannelSubscription filtered by channel and client") {
+                    let params = [
+                        "clientId": "tester",


probably better to reuse rest.options.clientId

funkyboy · 2018-04-03T15:20:01Z

Spec/PushChannel.swift

+                    expect { try channel.push.listSubscriptions(params) { _, _ in } }.to(throwError { (error: NSError) in
+                        expect(error.code).to(equal(ARTDataQueryError.invalidParameters.rawValue))
+                    })
+                }


Please add a test in which you actually subscribe and then you check that the result of listSubscriptions actually includes the result of the subscription.

Done 720d602 but it doesn't pass. I already reported that issue in Slack.

If you put a breakpoint on this line you'll get

(lldb) po channel.name "test-2-pushenabled:foo" (lldb)

@ricardopereira quite sure it's due to this https://github.com/ably/ably-ios/blob/push-channels/Spec/TestUtilities.swift#L100

Can you fix that please?

Fixed in 02b4d05

ricardopereira · 2018-05-04T13:07:52Z

Test should_return_a_paginated_result_with_PushChannelSubscription still fails with "40100: push not enabled for this channel"

ricardopereira · 2018-05-04T13:08:26Z

Rebased with push branch.

funkyboy · 2018-06-12T14:51:22Z

@ricardopereira when you pull down this branch to run it locally it throws a bunch of compilation errors

ricardopereira · 2018-06-12T17:33:01Z

@funkyboy You're working with Xcode 9.3?

funkyboy · 2018-06-12T17:48:13Z

@ricardopereira no, 9.2. Afair you said you didn’t upgrade any project settings.

ricardopereira · 2018-06-12T17:54:43Z

@funkyboy I did not update the project settings, I'm only using Xcode 9.3 #709 (comment)

funkyboy · 2018-06-12T17:58:21Z

@ricardopereira Yup, the point is that it doesn’t compile on 9.2 :(
Anyway an upgrade is on the way, so this will soon be a non problem.

ricardopereira · 2018-06-12T19:03:47Z

@funkyboy I think it's not a big deal. This won't affect users because it only affects tests and even so this is still in beta, from what I know.

funkyboy · 2018-06-14T14:34:56Z

@ricardopereira FYI I cherry-picked this commit 1845218 to both push and push-channels. Now Travis kicks in.

UPDATE: Sorry, I cherry-picked the commit ONLY to push. Feel free to propagate it as you see fit.

funkyboy · 2018-06-14T15:06:19Z

@ricardopereira also, there's a consistent failure on this line https://github.com/ably/ably-ios/blob/push-channels/Spec/PushActivationStateMachine.swift#L584

Likely due to the fact that the previous test doesn't have a
defer { rest.device.setAndPersistIdentityTokenDetails(nil) }

funkyboy · 2018-07-12T12:03:46Z

Rebased on push now.

NOTE: this commit will be dropped when this issue has been solved #735

ricardopereira requested a review from funkyboy March 23, 2018 15:34

ricardopereira force-pushed the push-device-auth branch 3 times, most recently from 4d514d0 to 3fe9980 Compare March 30, 2018 11:07

ricardopereira force-pushed the push-channels branch from 6567d54 to 3981416 Compare March 30, 2018 13:02

ricardopereira changed the base branch from push-device-auth to push-device-auth-tests March 30, 2018 13:02

ricardopereira changed the title ~~Push Channels~~ Push Channels tests Mar 30, 2018

funkyboy suggested changes Apr 3, 2018

View reviewed changes

ricardopereira force-pushed the push-channels branch from 72847f4 to 07ec07c Compare April 27, 2018 09:10

ricardopereira changed the base branch from push-device-auth-tests to push May 4, 2018 11:08

ricardopereira force-pushed the push-channels branch 3 times, most recently from 58633b7 to 3a7fa24 Compare May 4, 2018 13:03

funkyboy force-pushed the push-channels branch from 88b0350 to 3a7fa24 Compare June 14, 2018 14:46

ricardopereira added 6 commits July 12, 2018 14:02

PushChannel

dcdebf7

RSH4a

b4cd14e

RSH7b

c93c158

RSH7c

38024cc

RSH7d

b06f40e

RSH7e

793afc5

ricardopereira added 5 commits July 12, 2018 14:02

Code arrangement

270c0dc

Fix typo

27b4045

Fix error message

d7c36ae

Enhance RSH7e

2060dd8

Add resetDeviceOnceToken

78c1058

funkyboy force-pushed the push-channels branch from 5800868 to 78c1058 Compare July 12, 2018 12:03

funkyboy added 7 commits July 12, 2018 14:14

Update Travis to Xcode 9.4

7a26bb9

Simplify bad recipient error check in publish failure test

8fed6e3

Fix list subscription test

801d763

Use same variable for pushenabled:qux

5f423e7

Fix channel name wrongly prefixed in push notification test

02b4d05

Test: fix check for invalid response in realtime connection

783d307

Point to push-device-auth-dev instead of sandbox

117367c

NOTE: this commit will be dropped when this issue has been solved #735

funkyboy force-pushed the push-channels branch from b170fd1 to 117367c Compare July 12, 2018 15:13

funkyboy mentioned this pull request Jul 12, 2018

Push Activation State Machine missing tests #720

Merged

funkyboy merged this pull request into push Jul 13, 2018

funkyboy deleted the push-channels branch July 13, 2018 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push Channels tests #708

Push Channels tests #708

ricardopereira commented Mar 23, 2018

funkyboy left a comment

funkyboy Apr 3, 2018

ricardopereira Apr 6, 2018

funkyboy Apr 3, 2018

ricardopereira Apr 6, 2018

funkyboy Apr 3, 2018

ricardopereira Apr 6, 2018

funkyboy Apr 3, 2018

funkyboy Apr 3, 2018

ricardopereira Apr 20, 2018

funkyboy Jun 13, 2018

funkyboy Jun 13, 2018 •

edited

Loading

funkyboy Jul 12, 2018

ricardopereira commented May 4, 2018 •

edited

Loading

ricardopereira commented May 4, 2018

funkyboy commented Jun 12, 2018

ricardopereira commented Jun 12, 2018

funkyboy commented Jun 12, 2018

ricardopereira commented Jun 12, 2018

funkyboy commented Jun 12, 2018

ricardopereira commented Jun 12, 2018

funkyboy commented Jun 14, 2018 •

edited

Loading

funkyboy commented Jun 14, 2018

funkyboy commented Jul 12, 2018

Push Channels tests #708

Push Channels tests #708

Conversation

ricardopereira commented Mar 23, 2018

funkyboy 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

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

funkyboy Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardopereira commented May 4, 2018 • edited Loading

ricardopereira commented May 4, 2018

funkyboy commented Jun 12, 2018

ricardopereira commented Jun 12, 2018

funkyboy commented Jun 12, 2018

ricardopereira commented Jun 12, 2018

funkyboy commented Jun 12, 2018

ricardopereira commented Jun 12, 2018

funkyboy commented Jun 14, 2018 • edited Loading

funkyboy commented Jun 14, 2018

funkyboy commented Jul 12, 2018

funkyboy Jun 13, 2018 •

edited

Loading

ricardopereira commented May 4, 2018 •

edited

Loading

funkyboy commented Jun 14, 2018 •

edited

Loading