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

Upgrade to Socket io 1.3 #1404

Closed

Conversation

dignifiedquire
Copy link
Member

This is very much a work in progress, but from my tests neither the PR in #1257 nor #1258 does actually work. I hope to finish this in the next couple of days.

Update1: This only works when manually pulling in the forked & built version of karma-jasmine@0.1.5 that's why travis will be complaining, until we updated the release there.

Update2: Finally all tests passing locally :)

Update3: Update 1 is now obsolete as I switched the client tests to mocha.

@maksimr
Copy link
Contributor

maksimr commented May 15, 2015

@dignifiedquire Awesome!! :)

@dignifiedquire dignifiedquire changed the title [WIP] Upgrade to Socket io 1.3 Upgrade to Socket io 1.3 May 15, 2015
@dignifiedquire
Copy link
Member Author

@maksimr and everyone else, basics are working for me, now I need some more eyes on this to test it out please.

@maksimr
Copy link
Contributor

maksimr commented May 15, 2015

@dignifiedquire check in our projects all work fine(:ok_hand:) with latest version of karma-jasmine, karma-mocha, karma-webpack, phantomjs, Firefox, Chrome ;)

@dignifiedquire
Copy link
Member Author

@maksimr great. Also I was tired of jasmine so I switched the client tests to run on mocha with browserify :)

@maksimr
Copy link
Contributor

maksimr commented May 16, 2015

LGTM

@dignifiedquire dignifiedquire mentioned this pull request May 16, 2015
19 tasks
@dignifiedquire
Copy link
Member Author

@nikku @bendrucker do you have any ideas why this is failing? All is running fine on my local machine (osx + FF) but it seems travis doesn't like the setup :(

@bendrucker
Copy link

karma-browserify has a hack to add a preprocessor for the bundle file:

https://github.com/nikku/karma-browserify/blob/5d75d41879a32cc5a8b62b7c821b1ddbd796e870/lib/preprocessor.js#L3
https://github.com/nikku/karma-browserify/blob/5d75d41879a32cc5a8b62b7c821b1ddbd796e870/lib/preprocessor.js#L35

I'm betting the Karma under test !== the Karma that gets required on line 2. Try manually implementing the result from line 35 when you create your Karma config, adding: '**/*': ['browserify-bundle'] to config.preprocessors.

@dignifiedquire
Copy link
Member Author

Thanks a lot @bendrucker that fixed it :)

@bendrucker
Copy link

Awesome. I'm guessing there's a reason npm doesn't handle that peer dep more cleverly. Couldn't find it though.

@dignifiedquire dignifiedquire force-pushed the socket-io-1.3 branch 4 times, most recently from 823e888 to 47d04fa Compare May 17, 2015 11:28
@dignifiedquire
Copy link
Member Author

@bendrucker looks like that wasn't actually the issue, travis started failing after one successful run again. The issue is that we are deleting node_modules/karma but didn't symlink it, so karma-browserify couldn't find any installation of karma.

@dignifiedquire
Copy link
Member Author

@maksimr need a new review, needed more fixes for tests including karma-runner/integration-tests#4 to fix the integration tests.

@lygstate
Copy link

it's better not use 1.3.5, use 1.3 should be better, cause only the master
compat with iojs 2.1.0

@lygstate
Copy link

Ping for review and pull

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@dignifiedquire
Copy link
Member Author

Merged as 79072ae

self._onProcessExit(code, errorOutput);
});

self._process.on('error', function(err) {
self._process.on('karma_error', function(err) {

Choose a reason for hiding this comment

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

This line does not look correct. The error event in this case is coming from the child process, indicating that spawn failed. It is different from the error event emitted by the socket, which was renamed from "error" to "karma_error".

Copy link
Member Author

Choose a reason for hiding this comment

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

You are totally right, thanks a lot fixed in 45a6922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants