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

wip: fix windows bugs #1548

Merged
merged 3 commits into from Apr 8, 2014
Merged

wip: fix windows bugs #1548

merged 3 commits into from Apr 8, 2014

Conversation

@nlf
Copy link
Member

@nlf nlf commented Apr 3, 2014

This is where I'm at so far. The remaining problems are inconsistent, and all timeout related

  • test/server.js id 788 'Server Timeouts return server error message when server taking too long'
  • test/server.js id 791 'Server Timeouts does not return an error response when server is slow but faster than timeout'
  • test/clientTimeout.js id 44 'Client Timeout does not return a client error message when response is taking a long time to send'
  • test/proxy.js id 333 'Proxy times out when proxy timeout is less than server'

I have no idea at all how to fix these ones, so any hints would be appreciated.

@@ -1346,7 +1346,7 @@ describe('Response', function () {
});
});

it('closes file handlers when not reading file stream', function (done) {
it('closes file handlers when not reading file stream', { skip: process.platform === 'win32' }, function (done) {
Copy link
Member Author

@nlf nlf Apr 3, 2014

this test got disabled because windows has no lsof command, so the childprocess stream doesn't work

@nlf
Copy link
Member Author

@nlf nlf commented Apr 3, 2014

Common themes that we could consider moving to some utility functions are to create isAbsolutePath and isDirectoryPath methods, since the majority of the fixing was replacing checks that asserted either the first or last character of a path were '/'

@nlf
Copy link
Member Author

@nlf nlf commented Apr 3, 2014

Totally didn't notice #1547 but that's a better start to this. It does need a bit more, but I think that should get merged first and then this pull request updated.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 4, 2014

Does #1547 change any of these modifications?

@nlf
Copy link
Member Author

@nlf nlf commented Apr 4, 2014

Yes, it'll make it easy to clean things up if that one gets merged first. —
Nathan LaFreniere

On Thu, Apr 3, 2014 at 11:03 PM, Eran Hammer notifications@github.com
wrote:

Does #1547 change any of these modifications?

Reply to this email directly or view it on GitHub:
#1548 (comment)

@nlf
Copy link
Member Author

@nlf nlf commented Apr 7, 2014

Now that #1547 was merged I'll get this updated

@nlf
Copy link
Member Author

@nlf nlf commented Apr 7, 2014

Ok, this is now down to the above mentioned timing issues.. All of which are inconsistent. One pass they run, next pass they're off by a fairly large margin.

@hueniverse hueniverse added the bug label Apr 8, 2014
@hueniverse hueniverse added this to the 4.0.0 milestone Apr 8, 2014
@hueniverse hueniverse self-assigned this Apr 8, 2014
hueniverse pushed a commit that referenced this issue Apr 8, 2014
@hueniverse hueniverse merged commit c872f5e into master Apr 8, 2014
1 check failed
@nlf nlf deleted the windows branch Apr 8, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants