Skip to content

Fix a few assertions are not executed properly #1999

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

Merged
merged 1 commit into from
Nov 22, 2015

Conversation

nkzawa
Copy link
Contributor

@nkzawa nkzawa commented Feb 11, 2015

No description provided.

@@ -636,11 +636,10 @@ describe('socket.io', function(){
});
function getClients() {
sio.of('/chat').clients(function(error, sids) {
expect(error).to.be.undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it's cleaner to just do

expect(error).to.be(undefined)

instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't matter either way for me. But the value of error is actually null, so .to.not.be.ok() would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Aye.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and rebased.

@nkzawa
Copy link
Contributor Author

nkzawa commented Feb 12, 2015

umm, why the ci build failed?

@nkzawa
Copy link
Contributor Author

nkzawa commented Feb 14, 2015

I restarted the build and it's fine now.

rauchg added a commit that referenced this pull request Nov 22, 2015
Fix a few assertions are not executed properly
@rauchg rauchg merged commit 988d9d2 into socketio:master Nov 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants