RTL6g #205

ricardopereira · 2016-02-10T12:27:24Z

❌ Not passing.

Can't publish a message of type ARTMessage: channel.publish(message, ...)

2016-02-10 15:31:37.259 xctest[4649:209394] ERROR: ARTChannel: error encoding data, status: 2029696992
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/ablySpec/RealtimeClientChannel.swift:560: error: -[ablySpec.RealtimeClientChannel Channel__publish__Identified_clients_with_clientId__When_publishing_a_Message_with_clientId_set_to_null__should_be_unnecessary_to_set_clientId_of_the_Message_before_publishing_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRealtimeClientChannelswift_536] : failed: caught "NSInvalidArgumentException", "message encoding failed"
(
    0   CoreFoundation                      0x00b5ea94 __exceptionPreprocess + 180
    1   libobjc.A.dylib                     0x0061fe02 objc_exception_throw + 50
    2   ably                                0x0a5a4817 -[ARTChannel encodeMessageIfNeeded:] + 487
    3   ably                                0x0a5a613b __41-[ARTRealtimeChannel publishMessages:cb:]_block_invoke + 91
    4   ably                                0x0a5ea357 -[NSArray(ARTFunctional) artMap:] + 455
    5   ably                                0x0a5a6012 -[ARTRealtimeChannel publishMessages:cb:] + 370
    6   ably                                0x0a5a5e2c -[ARTRealtimeChannel publish:withName:cb:] + 268
    7   ably                                0x0a5a5cde -[ARTRealtimeChannel publish:cb:] + 318

It is possible in JavaScript:

channel.publish({ data: 'boom!', name: 'event' });

mattheworiordan · 2016-02-10T13:01:01Z

ablySpec/RealtimeClientChannel.swift

+                        pending("should be unnecessary to set clientId of the Message before publishing") {
+                            let options = AblyTests.commonAppSetup()
+                            options.autoConnect = false
+                            options.clientId = "client_string"


This test says "When publishing a Message with clientId set to null" yet you are setting the clientId?

@mattheworiordan Ok, I see. A little confusing because the RTL6g says Identified clients with a clientId... I thought the client needs a clientId but the message is published without a clientId (clientId attribute set to null) like I did:

let message = ARTMessage(data: "message", name: nil) expect(message.clientId).to(beNil())

I will fix the test.

The spec says "Identified clients with a clientId (as a result of either an explicitly configured clientId in ClientOptions, or implicitly through Token Auth)", but this client doesn't have a clientId, does it?

@tcard I removed the clientId ^

ricardopereira · 2016-02-12T15:23:57Z

ablySpec/RealtimeClientChannel.swift

+                            expect(transport.protocolMessagesSent.filter { $0.action == .Message }).toEventually(haveCount(1), timeout: testTimeout)
+
+                            let messageSent = transport.protocolMessagesSent.filter({ $0.action == .Message })[0]
+                            expect(messageSent.messages![0].clientId).to(beNil())


Failing with: failed - expected to be nil, got <>

ricardopereira · 2016-02-15T19:01:18Z

Rebased with changes.

tcard · 2016-02-16T16:28:08Z

ablySpec/RealtimeClientChannel.swift

+                        pending("should be unnecessary to set clientId of the Message before publishing") {
+                            let options = AblyTests.commonAppSetup()
+                            options.autoConnect = false
+                            let client = ARTRealtime(options: options)


The spec says "Identified clients with a clientId (as a result of either an explicitly configured clientId in ClientOptions, or implicitly through Token Auth)", but this client doesn't have a clientId, does it?

~~Done~~
I mean, see #205 (comment)

This is still wrong as far as I can see. This is supposed to be testing publishing a message without an explicit clientId, yet the client must be identified i.e. with a clientId. This client is not identified

@mattheworiordan @ricardopereira I've seen the discussion that @ricardopereira linked, and isn't that just what was done before? The client was being identified through ClientOptions.clientId.

I'm not following you unfortunately. This test needs an identified client to publish a message without specifying an explicit clientId for the message. Not much more to it than that.

Yes, but unless I'm reading the code very wrong, isn't that what @ricardopereira did initially and you asked him to change?

From the code snippet commented [here](8 days ago):

+ pending("should be unnecessary to set clientId of the Message before publishing") { + let options = AblyTests.commonAppSetup() + options.autoConnect = false + options.clientId = "client_string"

That's setting the clientId in the connection options, not in the message.

Yes, I had that before. The client with a valid clientId and the message published with no clientId.

If that's the case then I apologise, I misread it the first time. Bit confused why no one pulled me up on my mistake though!

I've tried to review the rest of the code beneath the options.clientId at the time of my comment and Github is not allowing me to see that, so it's possible the message clientId was being set?

Anyway, setting a clientId is valid in the options, and setting one in the message is not valid, in the context of this test.

ricardopereira · 2016-02-19T00:47:24Z

Done

tcard · 2016-02-19T07:51:38Z

LGTM

ricardopereira · 2016-02-19T11:41:33Z

@mattheworiordan PTAL

RTL6g

Fix type of authCallback in IDL.

mattheworiordan reviewed Feb 10, 2016
View reviewed changes

ricardopereira force-pushed the RTL6g branch from 2c22c12 to 9cab4a3 Compare February 12, 2016 15:21

ricardopereira reviewed Feb 12, 2016
View reviewed changes

ricardopereira force-pushed the RTL6g branch from 9cab4a3 to 8bae968 Compare February 15, 2016 19:00

tcard reviewed Feb 16, 2016
View reviewed changes

ricardopereira force-pushed the RTL6g branch 2 times, most recently from 03ff9e1 to 8e14b9b Compare February 16, 2016 23:42

ricardopereira added 4 commits February 18, 2016 18:25

RTL6g1a

cfd33e9

RTL6g1a: pending

530b284

RTL6g1b

60d15d9

RTL6g1b: pending

744dec6

ricardopereira force-pushed the RTL6g branch from 8e14b9b to 744dec6 Compare February 18, 2016 18:29

ricardopereira added 3 commits February 18, 2016 22:47

Enhance ARTMessage: better Swift support

6b02737

Test suite: test all the possible CryptoTest

0c5f641

fixup! RTL6g1b

fb85c54

ricardopereira added a commit that referenced this pull request Feb 22, 2016

Merge pull request #205 from ably/RTL6g

1df2586

RTL6g

ricardopereira merged commit 1df2586 into master Feb 22, 2016

ricardopereira deleted the RTL6g branch February 22, 2016 18:29

maratal pushed a commit that referenced this pull request Jul 19, 2023

Merge pull request #205 from ably/auth-callback-idl

a9d67ca

Fix type of authCallback in IDL.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTL6g #205

RTL6g #205

ricardopereira commented Feb 10, 2016

mattheworiordan Feb 10, 2016

ricardopereira Feb 10, 2016

mattheworiordan Feb 12, 2016

ricardopereira Feb 16, 2016

ricardopereira Feb 12, 2016

ricardopereira commented Feb 15, 2016

tcard Feb 16, 2016

ricardopereira Feb 18, 2016

mattheworiordan Feb 18, 2016

tcard Feb 18, 2016

mattheworiordan Feb 18, 2016

tcard Feb 18, 2016

ricardopereira Feb 18, 2016

mattheworiordan Feb 18, 2016

ricardopereira commented Feb 19, 2016

tcard commented Feb 19, 2016

ricardopereira commented Feb 19, 2016

RTL6g #205

RTL6g #205

Conversation

ricardopereira commented Feb 10, 2016

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

ricardopereira commented Feb 15, 2016

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

ricardopereira commented Feb 19, 2016

tcard commented Feb 19, 2016

ricardopereira commented Feb 19, 2016