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

Random port assignment as opposed to OS port assignment & non-freedom generic tests #42

Open
iislucas opened this issue Dec 22, 2014 · 3 comments
Labels

Comments

@iislucas
Copy link

at: https://github.com/freedomjs/freedom-for-firefox/blob/master/spec/core.tcpsocket.unit.spec.js#L8

I notice we are manually choosing ports with:
var portNumber = Math.floor((Math.random() * 999) + 8001);

This will presumably results in flakey tests (although not terribly flakey as we have a 1000 values we randomly choose between) ? Shouldn't we instead be asking the OS to choose a port for us (by giving port number 0, and then looking up the port that was chosen for us)?

Also: aren't our tcp-socket tests supposed to be in freedom so that we run the same tests for all platforms? (nothing about these tests seems terribly firefox specific).

Thoughts?

@willscott
Copy link
Member

The platform-independent / functional TCP tests do live in freedom: https://github.com/freedomjs/freedom/blob/master/spec/providers/coreIntegration/tcpsocket.integration.src.js

The ones here were added as platform-specific unit tests as we were building this provider iirc, the specific implementation has clientSocket and serverSocket classes, and these tests provided some minimal interaction that can be used to get at those classes individually without debugging through and reasoning about the freedom calls.

@willscott
Copy link
Member

On first glance these look to largely be duplicate to the platform-level tests, but I don't see much harm in keeping them. Do you notice areas here that test cases we don't have at the platform level and should refactor there?

I don't think we've seen these tests to be particularly unstable, so I don't see the random thing as something that needs immediate attention, maybe you have?

@iislucas
Copy link
Author

I don't think it needs immediate attention. Was just noticing it looked a little funny and thought it would be wise to have a placeholder. Probably worth adding to the header that these tests are really for debugging and demonstrating how to write platform-specific tests. I think it's a good idea to have them as they work as a useful demonstration and will probably help people debugging.

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

No branches or pull requests

2 participants