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

Get Travis CI passing consistently #85

Merged
merged 55 commits into from
Jan 5, 2016
Merged

Conversation

ricardopereira
Copy link
Contributor

Fixed:

  • Domain based on the testing environment.
  • if algorithm is nil then cipherEncoder should be ignored.
  • RTC5b.
  • RSN4: channel as a weak reference but the pointer does not mutate. The object is deallocated but it does not zero out the pointer.

@mattheworiordan
Copy link
Member

Looks good, would be nice (if available) to have tests marked as pending so that we see this in the test output.

@ricardopereira ricardopereira mentioned this pull request Dec 3, 2015
2 tasks
@ricardopereira
Copy link
Contributor Author

Rebase from master.

@ricardopereira
Copy link
Contributor Author

@mattheworiordan Passing.

@mattheworiordan
Copy link
Member

I change it to pending because it does not work on the test server. I need to investigate more.

Fine, however I would like to know why it isn't passing as well. I think it would make sense, where possible, to fix issues as and when we find them, as opposed to defer all the failing tests till the end of the process. That way the pain is spread out, what do you think?

PS. If you enable a very verbose log level and send us the raw protocol and REST communication, we can confirm if the issue is on our side, and if not, help you to determine what may be wrong on your side

@ricardopereira
Copy link
Contributor Author

  • Created a script run-tests to be sure we run the same commands on ci server and local machine
  • It is possible to choose which build tool to perform the tests: env BUILDTOOL
    • Options: xctool (default), xcodebuild-pretty, xcodebuild-travis, xcodebuild
    • Example: BUILDTOOL=xcodebuild ./Scripts/run-tests.sh
  • It is possible to run only one test class like: CLASS=ablyTests:ARTRealtimePresenceHistoryTest ./Scripts/run-tests.sh
  • Changed main scheme to run tests on the same build (the old way was runnning Swift tests and then ObjC tests. Now it runs both on the same execution)
  • Before testing, does a clean command

@mattheworiordan
Copy link
Member

@ricardopereira not quite sure what this PR is about anymore, it's titled stats.
Also, the tests are not passing on CI either.

@mattheworiordan mattheworiordan changed the title Swift test: Stats Swift test: Get Travis CI passing consistently Dec 4, 2015
@mattheworiordan mattheworiordan changed the title Swift test: Get Travis CI passing consistently Get Travis CI passing consistently Dec 4, 2015
@ricardopereira
Copy link
Contributor Author

Just for the record - some connections remain active after the test:

RealtimeClient__options__should_support_the_same_options_as_the_Rest_client_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRealtimeClientswift_34]' started.
2015-12-21 07:11:13.533 xctest[14418:4191633] DEBUG: (ARTWebSocketTransport.m:282) 0x7cfc9300 websocket did receive message {"action":8} <---- From another test
2015-12-21 07:11:13.533 xctest[14418:4191633] DEBUG: (ARTWebSocketTransport.m:220) 0x7cfc9300 websocket did disconnect (code 1000) 
2015-12-21 07:11:15.720 xctest[14418:4191584] DEBUG: (ARTRest.m:72) 0x7cef8d40 alloc HTTP
2015-12-21 07:11:15.720 xctest[14418:4191584] DEBUG: (ARTAuth.m:71) setting up auth method Token with key
2015-12-21 07:11:15.721 xctest[14418:4191584] DEBUG: (ARTRest.m:86) initialised 0x7c86a2c0

screen shot 2015-12-21 at 07 55 42


Please visit https://support.ably.io/ for access to our knowledgebase and to ask for any assistance.
To see what has changed in recent versions of Bundler, see the [CHANGELOG](CHANGELOG.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bundler?

@tcard
Copy link
Contributor

tcard commented Dec 28, 2015

LGTM, FWIW :)

@ricardopereira
Copy link
Contributor Author

@mattheworiordan I forgot to mention but I finished the remaining work.

@tcard
Copy link
Contributor

tcard commented Jan 4, 2016

Sorry to interrupt with this meta-issue, but in every team I've worked with before we tried to keep pull requests focused, atomic and as minimal as possible, to facilitate review, deployment, possible reverts, etc. But I keep seeing new commits in this one, e. g. a1272d9 changing the license, that really depart from this view. In those other teams I would've been asked to close this pull request and make smaller ones, with the commits adequately arranged and squashed for each one. I guess we can spare that to avoid even more distraction, but at least I think we should stop doing it from now on, unless in this team we want to work different to this for some reason.

It is especially hard for me to review this this way, because I'm new and I'm getting used to the codebase and the specs.

@ricardopereira
Copy link
Contributor Author

@tcard This PR is really a mess and it is my fault. It should not be that way. The license has changed on the master but then I did a merge where it was a mistake and now I revert it again. I believe you are struggling with this PR but it will never happen again. I can clarify you anything. I am happy to help.

@mattheworiordan I already reviewed the missing comments.

@tcard
Copy link
Contributor

tcard commented Jan 4, 2016

OK, I've seen now the comments about merging master. That was really messy. E. g. we have b5695fc in this PR, then reverted by the merge master, then a1272d9f28a698c6cda06f0092516be12d035a27 doing exactly the same thing.

Can we rebase -i this at least to remove the master merge?

@ricardopereira
Copy link
Contributor Author

@tcard Yes, I tried to revert b78ac4f but there were necessary changes so I chose to recommit the missing parts.

…d: RTC5b.

RSN4: channel as a weak reference but the pointer does not mutate. The
object is deallocated but it does not zero out the pointer.
…cripts: run-tests.sh and setup.sh

 - Created a script `run-tests` to be sure we run the same commands on ci server and local machine
 - It is possible to choose which build tool to perform the tests: env BUILDTOOL
     Options: xctool (default), xcodebuild-pretty, xcodebuild-travis, xcodebuild
     Example: BUILDTOOL=xcodebuild ./Scripts/run-tests.sh
 - It is possible to run only one test class like: `CLASS=ablyTests:ARTRealtimePresenceHistoryTest ./Scripts/run-tests.sh`
 - Changed main scheme to run tests on the same build (the old way was runnning Swift tests and then ObjC tests. Now it runs both on the same execution)
 - Before testing, does a `clean` command
 - Run setup.sh to install tools dependencies
@tcard tcard force-pushed the swift-tests-fix-stats branch from b11cd83 to 3529066 Compare January 4, 2016 12:40
@tcard
Copy link
Contributor

tcard commented Jan 4, 2016

I've done the rebase -i together with @ricardopereira and now it's a bit less messy. However, when rebasing on top of master I had to handle conflicts again and I probably messed up something, so then I had to add 1df3e28. Sorry, it's not ideal, but at this point I think it isn't worth it to clean it up further.

@tcard
Copy link
Contributor

tcard commented Jan 4, 2016

(By the way, the changes in the license and README got lost because they probably shouldn't have been there to begin with. They came from commits outside this pull request, like b5695fc, but that also are not on master. They really should be merged to master separately.)

@ricardopereira
Copy link
Contributor Author

@tcard Seems everything fine. I messed up the license & readme with this 9731ecc 😓 I will create a PR with those changes. Thanks again. I learned one more thing about git.

@mattheworiordan
Copy link
Member

I think we should merge this so that this crazy PR can land and we can return to normality :)

👍

ricardopereira added a commit that referenced this pull request Jan 5, 2016
Get Travis CI passing consistently + SwiftWebSocket
@ricardopereira ricardopereira merged commit 9a600e6 into master Jan 5, 2016
@ricardopereira ricardopereira deleted the swift-tests-fix-stats branch January 5, 2016 13:43
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