Closed gvas closed 9 years ago
Would you say this fixes the issue for good? Any idea where the issue arrises? You think it's just that sometimes connections drop? We were thinking that maybe it has to do with running sauce tunnel, maybe the tunnel closes before the last test finishes.
I'm not 100% sure.
I suspect that these node.js commits may relate to the error: joyent/node@14a4245 joyent/node@88686aa
The first one is in node since v0.9.10, the second one since v0.10.3. I'll run tests against versions where none, one and both of the commits are applied.
Until then, could you, @LaurentGoderre or @cvrebert monkey patch your grunt-saucelabs package and run your tests locally? And see if the situation improves? The monkey patching consists of
I will be able to do that on Thursday, is that ok?
I wonder if we can split this into two PR, the first two commits are good even if the rest doesn't fix the problem
Sure, I will put the logging improvements into a separate PR if the fix does not work.
Speaking of which, I have run my test suite (98 tests) a couple of times under different node.js
versions with both the request and the requestretry package. Under node.js v0.8.25
each test run finished successfully, with both the request
and the requestretry
packages. Under node.js v0.10.2
there were weird ECONNRESET errors with both the request
and the requestretry
packages, which even requestretry
could not handle. Under node.js v0.10.3
with the request
package each test run failed with ECONNRESET. With the requestretry
package each test run succeeded.
node.js | request | requestretry |
---|---|---|
0.8.25 | :white_check_mark: | :white_check_mark: |
0.10.2 | :no_entry_sign: | :no_entry_sign: |
0.10.3 | :no_entry_sign: | :white_check_mark: |
0.10.35 | :no_entry_sign: | :white_check_mark: |
It seems that these ECONNRESET errors are indeed transient network errors, which were swallowed in node.js
before v0.9.10. After v0.9.10 these errors surfaced, but before v0.10.3 there was some bug in node.js
which prevented requestretry
to handle these.
In short, this PR fixes the ECONNRESET errors under node v0.10.3+.
I think it's fair to require to ask user to upgrade to node 10.3. My only concern is with Travis and other CI, are they already up to date?
Travis is up to date!
@gvas I find it simpler to clone the repo, checkout the PR as a branch than use npm link to test ;)
Wow, awesome work. Going to merge this and publish a new version. Will add your handy graph to the readme.
@LaurentGoderre: that's a better method, indeed. I didn't know npm link. What were the results of your tests?
@Jonahss: Thanks. Are you sure about inserting the table to the readme? The request column will be irrelevant to the users, because the library will use requestretry after merging this PR. Maybe a warning that node 0.10.0, 0.10.1, 0.10.2 may cause problems would be sufficient.
The error was intermittent so I ran 20 or so tests and it didn't fail with this specific error. It looks good to me
@gvas Are you able to run the tests? I'm having trouble with running "sauce_tunnel:server"
These fixes were able to give WinJS its first clean test run since December 16.
@Jonahss : My apologies, I have missed the email for your comment. I fixed the tests, could you give them another try?
Awesome.
Thank you from noflo/noflo-ui!!! :bow:
Before this, I'd resigned to only running Sauce on Saturdays :grimacing:
Fix for the ECONNRESET errors. This PR: