Issues noticed while reviewing tests marked as done

[tcard]

I've kept a list of notes about different. Some of them are just questions, some of them are are things to be done. I've put a checkbox along with each one to keep track of what's been addressed.

• RSC1
  • Says "key, token", but iOS only takes key, and it's that way in the docs, but Ruby takes both and discriminates with regexp. (Update: it should be done per the spec.)
• RSC7a
  • Why is it grey?
• RSC10
  • "Using the Auth class a token should be automatically issued if possible, otherwise it should result in an error" The "otherwise" case isn't tested.
• RSC14b
  • Is it the same as RSA4? It is linked as such. If so, maybe it should be removed.
• RSL1d
  • Untested, but it says "yes"
• RSL2b1
  • "start must be equal to or less than end and is unaffected by the request direction" That seems to be untested.
• RSL5b
  • The test code doesn't seem to be encrypting anything.
• RTC1c
  • I'm not sure the test is testing this at all, but I don't know how it could be tested.
• RTC1d
  • There's just a comment in the test. realtimeHost is read-only... why?
• RTC2
  • Says 'yes', but code says TODO.
• RTN4e
  • Says 'yes', but code says a TODO and has a comment that has nothing to do with this. (I've removed it.)
• RTS3a
  • Says 'yes', but Channels#get isn't even a thing. ARTRealtime.channels is a dictionary.
• RTS4a
  • Says 'yes', but method is ARTRealtime#removeAllChannels instead of Channels#release.
• RTL4d
  • Says 'yes', but the linked test is completely different. ARTRealtimeChannel.attach doesn't provide a callback.
• RTL5e
  • Same as RTL4d.
• RTL7c
  • Linked test has nothing to do with spec; the "however" case isn't tested.
• RTL7f
  • Again, linked test has nothing to do with spec. There's a testEchoMessagesFalse which also doesn't test this.
• RTP6a
  • Again, linked test has nothing to do with spec.
• RTP6c
  • "However" part isn't tested.
• RTP7b
  • Describes an "action" argument that seems to be implemented en iOS. However, it is not in the docs, nor in Java. In those, the method takes a PresenceListener; iOS seems to take both! In Ruby it does take an array of "actions". (Update: it should be done per the spec.)
• RTP8b
  • Failure isn't tested.
• RTP8d
  • Channel FAILED case isn't tested.
• RTP10a
  • It says: (RTP10a) "If the language permits the data argument to be omitted, then the previously set data value will be omitted" I think that last "omitted" should be "sent"? That's what's in the test anyway.
  • https://github.com/ably/ably-ios/blob/96720f7c03fa60c75f69b1d0384648248be499b2/ably-iosTests/ARTRealtimePresenceTest.m#L217 should be linked as well.
• RTP10d
  • Not exhaustive? It only tests failure when channel is detached. (Does Presence.leave detach implicitly? It doesn't seem to be documented.)
• RTP15d
  • Only success is tested.
• RTP15e
  • Again, linked test has nothing to do with spec.
• TS1
  • In the example JSON, there are "inProgress", "count", "unit" an "intervalId" keys while ARTStats only has a NSDate interval property.
• TS3
  • Ruby and Obj-C interfaces don't match at all.
• TS7
  • Has "push" and "httpStream" instead of "webhook".

[mattheworiordan]

RSC1: Says "key, token", but iOS only takes key, and it's that way in the docs, but Ruby takes both and discriminates with regexp.

Well spec says a token string or a key string, but does not specify a Token in the spec. It's a convenience we have added, see https://github.com/ably/wiki/issues/47 for proposed change.

RSC7a: Why is it grey?

Simply means no one has looked at it yet so it's not definitely no, wip, or yes i.e. someone needs to update the sheet with a value.

RSC10: "Using the Auth class a token should be automatically issued if possible, otherwise it should result in an error" The "otherwise" case isn't tested. (See note on RSC14c.)

So please mark the test as wip then so that we add suitable test coverage. In terms of RSC14c, I agree there is some overlap, however RSC10 is much broader whereas RSC14c specifically talks about the use case where a token or tokenDetails object is provided.

RSC14b: Is it the same as RSA4? It is linked as such. If so, maybe it should be removed.

True, in fact that whole (RSC14) Authentication section should really be moved to the RSA section as it does not belong in the RestClient section. I have modified the spec at ably/docs@c9d06f0, and added an issue ably/docs#78 for the 0.9 release

[tcard]

@mattheworiordan

Well spec says a token string or a key string, but does not specify a Token in the spec. It's a convenience we have added, see ably/wiki#47 for proposed change.

I'm not sure what you're saying. There's no Token type defined, I guess you're referring to TokenDetails? Anyway, the spec says a token string should be accepted by the AblyRest constructor, but that isn't implemented in iOS, and the docs just specify the constructors AblyRest(keyStr) and AblyRest(options). So as I see it either the docs should be expanded to match the spec by adding a AblyRest(tokenStr) constructor (and the iOS library updated as well), or the token string constructor should be removed from the spec.

As per accepting a TokenRequest in the AblyRest constructor, wouldn't you need to first instantiate an AblyRest to get the TokenRequest?

So please mark the test as wip then so that we add suitable test coverage.

Everything mentioned as untested has already been marked as WIP.

however RSC10 is much broader whereas RSC14c specifically talks about the use case where a token or tokenDetails object is provided.

OK, I've removed RSC14c from the list.

[mattheworiordan]

RSC14c; Seems to be the same as the "otherwise" case in RSC10. In that case, maybe it should be removed.

As with previous note, we will address this more fully in 0.9, however I have updated a bit to reduce duplication.

RSL1d: Untested, but it says "yes"

Well please change it to no then!

RSL2b1: "start must be equal to or less than end and is unaffected by the request direction" That seems to be untested.

As above, please change in the spreadsheet

RSL5b: The test code doesn't seem to be encrypting anything.

I don't see any test code?

RTC1c: I'm not sure the test is testing this at all, but I don't know how it could be tested.

Simply check the WebSocket endpoint and see if the query param exists

RTC1d: There's just a comment in the test. realtimeHost is read-only... why?

Not sure where you mean it's realtime, but in general, once a client library is instanced, the options like host or clientId are read-only, that is by design as we don't want people interfering with the configuration after it's established a connection. Is that what you mean?

RTC2: Says 'yes', but code says TODO.

Ok, please amend

RTN4e: Says 'yes', but code says a TODO and has a comment that has nothing to do with this. (I've removed it.)

Ok, please amend the sheet accordingly

RTS3a: Says 'yes', but Channels#get isn't even a thing. ARTRealtime.channels is a dictionary.
RTS4a: Says 'yes', but method is ARTRealtime#removeAllChannels instead of Channels#release.
RTL4d: Says 'yes', but the linked test is completely different. ARTRealtimeChannel.attach doesn't provide a callback.
RTL5e: Same as RTL4d.

Ok, please amend the sheet so with no then

RTL7c: Linked test has nothing to do with spec; the "however" case isn't tested.
RTL7f: Again, linked test has nothing to do with spec. There's a testEchoMessagesFalse which also doesn't test this.

Ok, please mark as no then

RTP6a; Again, linked test has nothing to do with spec.
RTP6c; "However" part isn't tested.

Ok, please mark as no or wip

RTP7b: Describes an "action" argument that seems to be implemented en iOS. However, it is not in the docs, nor in Java. In those, the method takes a PresenceListener; iOS seems to take both! In Ruby it does take an array of "actions".

I think action is described in the docs, see http://docs.ably.io/client-lib-development-guide/features/#RTP7b

The array of actions is a convenience, but not part of the spec for now

RTP8b: Failure isn't tested.
RTP8d: Channel FAILED case isn't tested.

Ok, please update the sheet with no and wip

RTP10a: It says: (RTP10a) "If the language permits the data argument to be omitted, then the previously set data value will be omitted" I think that last "omitted" should be "sent"? That's what's in the test anyway.

Thanks, fixed in ably/docs@d55b74c

https://github.com/ably/ably-ios/blob/96720f7c03fa60c75f69b1d0384648248be499b2/ably-iosTests/ARTRealtimePresenceTest.m#L217 should be linked as well.

Ok, please do that in the sheet

RTP10d: Not exhaustive? It only tests failure when channel is detached. (Does Presence.leave detach implicitly? It doesn't seem to be documented.)

Presence.leave does not detach implicitly, why did you think that?

RTP15d: Only success is tested.
RTP15e: Again, linked test has nothing to do with spec.

Ok, please update in the sheet to wip and no

TS1: In the example JSON, there are "inProgress", "count", "unit" an "intervalId" keys while ARTStats only has a NSDate interval property.

inProgress and count not technically needed, but no harm in exposing them, so please add to the spec. The other fields should be present.

TS3: Ruby and Obj-C interfaces don't match at all.

Ok, we need to flag in the sheet, but address later as stats is not important for now.

TS7: Has "push" and "httpStream" instead of "webhook".

Yes, that is very old, needs to be updated, please mark as no in the sheet

[mattheworiordan]

I'm not sure what you're saying. There's no Token type defined, I guess you're referring to TokenDetails?

Yes, TokenDetails, sorry

So as I see it either the docs should be expanded to match the spec by adding a AblyRest(tokenStr) constructor (and the iOS library updated as well), or the token string constructor should be removed from the spec.

Ah, you are referring to the client side documentation as opposed to the spec. If you see issues in the client side docs please flag separately, however the client side docs are not your reference point as they are out of date, only the spec is.

As per accepting a TokenRequest in the AblyRest constructor, wouldn't you need to first instantiate an AblyRest to get the TokenRequest?

Yes, but you could be passed a TokenRequest object from another library or even from a server that
serialises the object & deserialises it.

[tcard]

Also, @mattheworiordan , could you clarify this?

RTP7b: Describes an "action" argument that seems to be implemented en iOS. However, it is not in the docs, nor in Java. In those, the method takes a PresenceListener; iOS seems to take both! In Ruby it does take an array of "actions".

If I just stick to the spec, as you say, then I just should do what Ruby does. (Java is then outdated.) The spec says though that Presence#subscribe and Presence#unsubscribe can take "no arguments" when it at least takes a listener, right? Maybe that should be clarified. (Also, "listener" probably should be exactly defined.)

[tcard]

(By the way, sorry to have bothered you with stuff which clearly is just "yep, should be fixed". I should have separated the items in a "needs clarification" section and a "ready to be done" section.)

[tcard]

Not sure where you mean it's realtime, but in general, once a client library is instanced, the options like host or clientId are read-only, that is by design as we don't want people interfering with the configuration after it's established a connection. Is that what you mean?

OK, I see that, the matter is that the test is just this: https://github.com/ably/ably-ios/blob/master/ablySpec/RealtimeClient.swift#L115 @ricardopereira can you clarify?

Presence.leave does not detach implicitly, why did you think that?

Yeah, nevermind that. The test is in fact done: https://github.com/ably/ably-ios/blob/5e4def84ad28da10840d521dbb65424c6b31d531/ably-iosTests/ARTRealtimePresenceTest.m#L1006 It was just not properly linked. I've fixed that.

[ricardopereira]

OK, I see that, the matter is that the test is just this: https://github.com/ably/ably-ios/blob/master/ablySpec/RealtimeClient.swift#L115 @ricardopereira can you clarify?

ARTRealtime doesn't have a realtimeHost property. It was and it is always using ARTDefault.realtimeHost, so the current API doesn't allow you to set a custom realtimeHost. You can only change the environment.

[ricardopereira]

I will close this when everything is merged.