Fix spec failures #506

ricardopereira · 2016-09-20T16:00:09Z

Executed 541 tests, with 0 failures (0 unexpected) in 246.092 (247.119) seconds

Merge this before #505.

ricardopereira · 2016-09-20T16:08:10Z

Source/ARTJsonLikeEncoder.m

@@ -427,6 +427,7 @@ - (ARTTokenRequest *)tokenRequestFromDictionary:(NSDictionary *)input error:(NSE
    [_logger verbose:@"RS:%p ARTJsonLikeEncoder<%@>: tokenRequestFromDictionary %@", _rest, [_delegate formatAsString], input];

    if (![input isKindOfClass:[NSDictionary class]]) {
+        *error = [ARTErrorInfo createWithCode:ARTStateRequestTokenFailed message:@"input provided has an invalid TokenRequest"];


@tcard Do you agree with this? I add this check because an authUrl can return an empty object. The lib will try to use that empty object as a TokenRequest and then the callback isn't called because the request is nil.

I have some doubts mostly because the authUrl can return a token or a tokenDetails or even a tokenRequest. Maybe it's important to add a test checking a wrong use of authUrl on each case.

If the method that uses authUrl (this one) expects the return to not be nil, then it should be that method the one that checks it, not the decoder. Arguably a nil input being decoded to a nil TokenRequest is correct behavior.

@tcard 6b4fb0f.

ricardopereira · 2016-09-20T16:09:36Z

Spec/RealtimeClientPresence.swift

@@ -904,7 +904,7 @@ class RealtimeClientPresence: QuickSpec {
                            guard let error = error else {
                                fail("error expected"); done(); return
                            }
-                            expect(error.message).to(contain("Channel denied access based on given capability"))
+                            expect(error.message).to(contain("Invalid clientId for credentials"))


I looked at the lib and I found nothing that could change this behaviour. It seems to be an Ably service change. @tcard what do you think?

That looks like your test is doing something odd with the clientId and changing this error message only indicates that this test is no longer testing "should result in an error if the client does not have required presence permission". Can you enable verbose logging and send us the log to look at, I suspect you're sending an invalid clientId somewhere

@mattheworiordan Fixed a05d086.

ricardopereira · 2016-09-20T16:11:15Z

Spec/Auth.swift

@@ -2034,7 +2034,7 @@ class Auth : QuickSpec {

        describe("Reauth") {
            // RTC8
-            it("should use authorise({force: true}) to reauth with a token with a different set of capabilities") {
+            pending("should use authorise({force: true}) to reauth with a token with a different set of capabilities") {


@EvgenyKarkan The re-attach is failing because the channel is in the Failed state. I saw that RTC8 as some subspecs so I assumed that it isn't ready.

@ricardopereira not sure what you mean by some subspecs?

@mattheworiordan I mean RTC8 is composed by RTC8a1, RTC8a2, RTC8a3, etc. So the test should be complying those but now I noticed that RTC8 is a new spec from v0.9. I will fix #485, the one responsible for failing the test.

mattheworiordan

@ricardopereira I'd like to know why you are getting that error now as it appears to me that the test is broken not the realtime system

mattheworiordan · 2016-09-21T10:36:09Z

Spec/Auth.swift

@@ -2034,7 +2034,7 @@ class Auth : QuickSpec {

        describe("Reauth") {
            // RTC8
-            it("should use authorise({force: true}) to reauth with a token with a different set of capabilities") {
+            pending("should use authorise({force: true}) to reauth with a token with a different set of capabilities") {


@ricardopereira not sure what you mean by some subspecs?

mattheworiordan · 2016-09-21T10:37:56Z

Spec/RealtimeClientPresence.swift

@@ -904,7 +904,7 @@ class RealtimeClientPresence: QuickSpec {
                            guard let error = error else {
                                fail("error expected"); done(); return
                            }
-                            expect(error.message).to(contain("Channel denied access based on given capability"))
+                            expect(error.message).to(contain("Invalid clientId for credentials"))


That looks like your test is doing something odd with the clientId and changing this error message only indicates that this test is no longer testing "should result in an error if the client does not have required presence permission". Can you enable verbose logging and send us the log to look at, I suspect you're sending an invalid clientId somewhere

mattheworiordan · 2016-09-21T10:38:08Z

Spec/RealtimeClientPresence.swift

                    let channel = client.channels.get("cannotpresence")

                    waitUntil(timeout: testTimeout) { done in
                        channel.presence.update(nil) { error in
-                            expect(error!.message).to(contain("Channel denied access based on given capability"))
+                            expect(error!.message).to(contain("Invalid clientId for credentials"))


Same applies here, we should not commit this

mattheworiordan · 2016-09-21T10:38:15Z

Spec/RealtimeClientPresence.swift

                    let channel = client.channels.get("cannotpresence")

                    waitUntil(timeout: testTimeout) { done in
                        channel.presence.leaveClient("other", data: nil) { error in
-                            expect(error!.message).to(contain("Channel denied access based on given capability"))
+                            expect(error!.message).to(contain("Invalid clientId for credentials"))


As above, don't commit

tcard · 2016-09-21T14:34:22Z

Source/ARTJsonLikeEncoder.m

@@ -427,6 +427,7 @@ - (ARTTokenRequest *)tokenRequestFromDictionary:(NSDictionary *)input error:(NSE
    [_logger verbose:@"RS:%p ARTJsonLikeEncoder<%@>: tokenRequestFromDictionary %@", _rest, [_delegate formatAsString], input];

    if (![input isKindOfClass:[NSDictionary class]]) {
+        *error = [ARTErrorInfo createWithCode:ARTStateRequestTokenFailed message:@"input provided has an invalid TokenRequest"];


If the method that uses authUrl (this one) expects the return to not be nil, then it should be that method the one that checks it, not the decoder. Arguably a nil input being decoded to a nil TokenRequest is correct behavior.

ricardopereira · 2016-09-22T07:28:50Z

Test suite is failing because of the recent version of bundler (v1.13.1).
Bundler issue rubygems/bundler#4981.

… publishing acknowledgement

- sometimes was passing, sometimes not

…to avoid creating flaky tests

ricardopereira · 2016-09-26T10:03:27Z

@tcard @mattheworiordan Can I merge this?

* Test suite: add `testSuite_waitForConnectionToClose` * Fix testMultipleText_1000_10: increase timeout * Fix connection broken after connection is closed: should wait for the publishing acknowledgement * Fix spec failures (#506) * Test suite: add `testSuite_waitForConnectionToClose` * Fix testMultipleText_1000_10: increase timeout * A few fixes in tests. * Fix connection broken after connection is closed: should wait for the publishing acknowledgement * Fix spec tests: expects `done` completion closure to only be called once * Fix: decode each REST presence message data * Fix RSP5 * Fix: should error if authUrl request is returning incompatible data * Fix RTP8h, RTP9e, RTP10e: token with clientId * Fix RTN17e: already closed * RTC8: pending * Fix #117 test: add delay * Enhance RTP5b: wait for presence of Client2 - sometimes was passing, sometimes not * Fix InvalidNimbleAPIUsage: nested async expectations are not allowed to avoid creating flaky tests

ricardopereira commented Sep 20, 2016

View reviewed changes

ricardopereira mentioned this pull request Sep 20, 2016

Test Suite issues #469

Closed

mattheworiordan requested changes Sep 21, 2016

View reviewed changes

tcard suggested changes Sep 21, 2016

View reviewed changes

ricardopereira mentioned this pull request Sep 22, 2016

Fix: should attach if the channel is in the FAILED state #507

Merged

tcard approved these changes Sep 22, 2016

View reviewed changes

tcard force-pushed the fix-spec-failures branch from 148702f to e0b308c Compare September 22, 2016 09:38

tcard force-pushed the fix-legacy-tests-account-blocked branch from 8d162bf to 7be9767 Compare September 22, 2016 09:39

tcard force-pushed the fix-spec-failures branch from e0b308c to ef767c7 Compare September 22, 2016 11:15

tcard force-pushed the fix-legacy-tests-account-blocked branch from 7be9767 to 6ae50a1 Compare September 22, 2016 11:15

tcard and others added 3 commits September 22, 2016 13:51

A few fixes in tests.

73ca1d0

Test suite: add testSuite_waitForConnectionToClose

00901e1

Fix testMultipleText_1000_10: increase timeout

33d18a7

tcard force-pushed the fix-spec-failures branch from ef767c7 to 54f5856 Compare September 22, 2016 11:52

tcard force-pushed the fix-legacy-tests-account-blocked branch from 6ae50a1 to e8e6836 Compare September 22, 2016 11:54

ricardopereira added 5 commits September 22, 2016 13:56

Fix connection broken after connection is closed: should wait for the…

0b0b011

… publishing acknowledgement

Fix spec tests: expects done completion closure to only be called once

61c3b4c

Fix: decode each REST presence message data

4fd0f95

Fix RSP5

76e29d7

Fix: should error if authUrl request is returning incompatible data

a51116a

tcard force-pushed the fix-spec-failures branch from 54f5856 to e8a399d Compare September 22, 2016 11:57

ricardopereira added 6 commits September 26, 2016 09:14

Fix RTP8h, RTP9e, RTP10e: token with clientId

f5801e6

Fix RTN17e: already closed

0ce9f45

RTC8: pending

ad56876

Fix #117 test: add delay

afc03d9

Enhance RTP5b: wait for presence of Client2

2a560c5

- sometimes was passing, sometimes not

Fix InvalidNimbleAPIUsage: nested async expectations are not allowed …

62d3610

…to avoid creating flaky tests

ricardopereira force-pushed the fix-spec-failures branch from 68398ef to 62d3610 Compare September 26, 2016 08:15

ricardopereira merged commit 8bec768 into fix-legacy-tests-account-blocked Sep 27, 2016

ricardopereira deleted the fix-spec-failures branch September 27, 2016 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix spec failures #506

Fix spec failures #506

ricardopereira commented Sep 20, 2016

ricardopereira Sep 20, 2016

tcard Sep 21, 2016

ricardopereira Sep 22, 2016

ricardopereira Sep 20, 2016

mattheworiordan Sep 21, 2016

ricardopereira Sep 22, 2016

ricardopereira Sep 20, 2016

mattheworiordan Sep 21, 2016

ricardopereira Sep 22, 2016

mattheworiordan left a comment

mattheworiordan Sep 21, 2016

mattheworiordan Sep 21, 2016

mattheworiordan Sep 21, 2016

mattheworiordan Sep 21, 2016

tcard Sep 21, 2016

ricardopereira commented Sep 22, 2016

ricardopereira commented Sep 26, 2016

Fix spec failures #506

Fix spec failures #506

Conversation

ricardopereira commented Sep 20, 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

Choose a reason for hiding this comment

mattheworiordan 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

ricardopereira commented Sep 22, 2016

ricardopereira commented Sep 26, 2016