Add websocket JS tests and call onclose on failure #3229

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
@dralletje
Contributor

dralletje commented Oct 4, 2015

No description provided.

@dralletje

This comment has been minimized.

Show comment
Hide comment
@dralletje

dralletje Oct 4, 2015

Contributor

Made this pull request instead of #3186 because I didn't know how to rebase it correctly, sorry!
But I added JS tests here, not sure if that is done correctly though ( @cpojer )

Contributor

dralletje commented Oct 4, 2015

Made this pull request instead of #3186 because I didn't know how to rebase it correctly, sorry!
But I added JS tests here, not sure if that is done correctly though ( @cpojer )

@dralletje

This comment has been minimized.

Show comment
Hide comment
@dralletje

dralletje Oct 4, 2015

Contributor

And just for more feedback 😁 Is this what you were thinking of in #2984 @ide ?

Contributor

dralletje commented Oct 4, 2015

And just for more feedback 😁 Is this what you were thinking of in #2984 @ide ?

Add websocket JS tests and comply better to spec
- Use event-target-shim for .onopen and such
- Emit 'close' when emitting 'error'
- Set readyState to the right value initially and on 'close'
@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Oct 4, 2015

Collaborator

This is going to have conflicts after #2837 gets merged.

Collaborator

satya164 commented Oct 4, 2015

This is going to have conflicts after #2837 gets merged.

@dralletje

This comment has been minimized.

Show comment
Hide comment
@dralletje

dralletje Oct 4, 2015

Contributor

@satya164 Not really conflict, but indeed because of the WebSocketBase it'll cause problems.
Do you have time tomorrow anywhere to resolve the problems together? (I didn't get the tests to run for android just yet, so we could look at that too maybe)

Contributor

dralletje commented Oct 4, 2015

@satya164 Not really conflict, but indeed because of the WebSocketBase it'll cause problems.
Do you have time tomorrow anywhere to resolve the problems together? (I didn't get the tests to run for android just yet, so we could look at that too maybe)

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Oct 5, 2015

Collaborator

OK. I'll try.

Collaborator

satya164 commented Oct 5, 2015

OK. I'll try.

+'use strict';
+
+jest
+ .autoMockOff()

This comment has been minimized.

@cpojer

cpojer Oct 5, 2015

Contributor

why do you have to turn off auto-mocking? Can't you just remove this line and continue mocking NativeModules just like you do below?

@cpojer

cpojer Oct 5, 2015

Contributor

why do you have to turn off auto-mocking? Can't you just remove this line and continue mocking NativeModules just like you do below?

This comment has been minimized.

@dralletje

dralletje Oct 6, 2015

Contributor

@cpojer I wish I could, but I couldn't make that work (ran into all kinds of errors, even when I was dontMock'ing all the things I needed). If you want to have a try, go ahead :)

@dralletje

dralletje Oct 6, 2015

Contributor

@cpojer I wish I could, but I couldn't make that work (ran into all kinds of errors, even when I was dontMock'ing all the things I needed). If you want to have a try, go ahead :)

@christopherdro

This comment has been minimized.

Show comment
Hide comment
@christopherdro

christopherdro Dec 16, 2015

Contributor

Related #4803

Contributor

christopherdro commented Dec 16, 2015

Related #4803

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 17, 2016

@vjeux would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

ghost commented Mar 17, 2016

@vjeux would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 20, 2016

Contributor

@dralletje It looks like this needs rebasing now. Do you plan to continue working on this?

Contributor

mkonicek commented Mar 20, 2016

@dralletje It looks like this needs rebasing now. Do you plan to continue working on this?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
Contributor

mkonicek commented Mar 20, 2016

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Mar 24, 2016

Collaborator

@dralletje Can you rebase against master?

Collaborator

satya164 commented Mar 24, 2016

@dralletje Can you rebase against master?

dralletje added some commits Mar 31, 2016

Merge remote-tracking branch 'facebook/master' into websocket-close-o…
…n-error

# Conflicts:
#	Libraries/WebSocket/WebSocket.ios.js
#	Libraries/WebSocket/WebSocketBase.js
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 31, 2016

@dralletje updated the pull request.

@dralletje updated the pull request.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 31, 2016

Contributor

Closing this, if you want to continue working on it please create a new pull request that only contains the changes you intend to make. This one has over 1000 changed files.

Contributor

mkonicek commented Mar 31, 2016

Closing this, if you want to continue working on it please create a new pull request that only contains the changes you intend to make. This one has over 1000 changed files.

@mkonicek mkonicek closed this Mar 31, 2016

@dralletje

This comment has been minimized.

Show comment
Hide comment
@dralletje

dralletje Mar 31, 2016

Contributor

Yeah, I dont know how I screwed this thing up, hahahaha

Contributor

dralletje commented Mar 31, 2016

Yeah, I dont know how I screwed this thing up, hahahaha

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