Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Legacy tests: Sandbox ClientOptions #405

Merged
merged 3 commits into from
Apr 21, 2016

Conversation

ricardopereira
Copy link
Contributor

XCTestExpectation was created before the https://sandbox-rest.ably.io:443/apps request and the timeout was easily exceeded.

@tcard
Copy link
Contributor

tcard commented Apr 19, 2016

LGTM, good one!

@ricardopereira ricardopereira force-pushed the testsuite-legacy-sandbox-clientoptions branch 4 times, most recently from daf1f58 to ecd3231 Compare April 19, 2016 13:44
@ricardopereira
Copy link
Contributor Author

⚠️ Please, do not merge.

Tests are still failing, so I decided to test the Travis network. I created this repo and added it to Travis. The internet connection is not slow (65.2 MB/s ⚡️). The build#2 proves that something weird is happening on the backend when a client is requesting a new snadbox app where sometimes it takes more than 10 seconds to retrieve an answer. I also tried to add more time for the https://sandbox-rest.ably.io:443/apps request and sometimes it takes more than 16 seconds to complete the request:

Test Case '-[ARTRealtimeMessageTest testSubscribeToName]' started.
2016-04-19 13:57:52.573 xctest[1659:6783] START sandbox app request +[ARTTestUtil setupApp:withDebug:withAlteration:appId:callback:] 2016-04-19 13:57:52 +0000
2016-04-19 13:58:09.119 xctest[1659:7948] END sandbox app request __64+[ARTTestUtil setupApp:withDebug:withAlteration:appId:callback:]_block_invoke 2016-04-19 13:58:09 +0000 (elapsed 16.546053)
Test Case '-[ARTRealtimeMessageTest testSubscribeToName]' passed (21.156 seconds).

The build#2193 last 48 min 16 sec and the test suite didn't finished (The job exceeded the maxmimum time limit for jobs, and has been terminated.).

I'm going back to the specs list.

@paddybyers Does this make any sense? Can you take a look at the backend please? /cc @mattheworiordan @tcard

@mattheworiordan
Copy link
Member

@ricardopereira but we should only ever be using one app, so why is 16s even an issue? Are we not sharing apps across the test suite?

@ricardopereira
Copy link
Contributor Author

@mattheworiordan Every test is requesting a new app. Wrong approach then?

@mattheworiordan
Copy link
Member

@mattheworiordan Every test is requesting a new app. Wrong approach then?

Definitely, we should fix that. In pretty much all other suites, we create one app and share that app for all tests as creating an app is expensive and by ensuring tests run on their own unique channels, there is no risk of overlap.

I believe the only exception to this is in some cases for stats tests as other activity can effect stats. So please go ahead and make that change, it will drastically improve performance.

@ricardopereira
Copy link
Contributor Author

... it will drastically improve performance.

Yes it will, indeed!

@tcard
Copy link
Contributor

tcard commented Apr 21, 2016

@ricardopereira I'll be focusing on iOS in the following days, so I'll tackle this first.

@ricardopereira
Copy link
Contributor Author

@tcard Thanks 👍

@ricardopereira
Copy link
Contributor Author

Please remove ecd3231. I was just testing on Travis.

ricardopereira and others added 3 commits April 21, 2016 16:51
 - XCTestExpectation was created before the sandbox app request and the
timeout was easily exceeded
@tcard tcard force-pushed the testsuite-legacy-sandbox-clientoptions branch from ecd3231 to 001b52f Compare April 21, 2016 14:53
@tcard tcard merged commit b44861e into master Apr 21, 2016
@mattheworiordan mattheworiordan deleted the testsuite-legacy-sandbox-clientoptions branch April 21, 2016 16:21
tcard pushed a commit that referenced this pull request May 16, 2016
* Test suite: newSandboxApp

* Use of `newSandboxApp`

 - XCTestExpectation was created before the sandbox app request and the
timeout was easily exceeded

* Fix compiler warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants