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

Remove the skipping of "should support sockets" test #3364

Merged

Conversation

@timemachine3030
Copy link
Contributor

@timemachine3030 timemachine3030 commented Oct 25, 2020

This PR re-enables the 'should support sockets' test. After review, the test is effective and passes an npm test run.

If a reason exists that this test is not effective or otherwise should not be included in the test run please explain in a comment so I can take additional steps to correct the test or remove it.

Thanks!

This pull request is the result of an in-depth code review I did on this package. You can read the complete review on my blog, http://ctrl-c.club/~timemachine/2020/10/24/code-review-axios/

Pilot and others added 3 commits Oct 25, 2020
@jasonsaayman
Copy link
Collaborator

@jasonsaayman jasonsaayman commented Oct 28, 2020

@timemachine3030 thanks for the pull request, don't know why that test was skipped. By the way, your code review on axios is awesome, a great read! @chinesedfan maybe you know why this test was set to skip? If not let's add it back.

@jasonsaayman jasonsaayman added this to the v0.21.1 milestone Oct 28, 2020
@jasonsaayman jasonsaayman self-assigned this Oct 28, 2020
Copy link
Collaborator

@chinesedfan chinesedfan left a comment

@jasonsaayman It was skipped by #1655 and I don't know why, either. No reason to refuse adding it back.

@timemachine3030 Really shocked by the detailed and professional code review, which also includes npm audit and test coverage. Some problems are known to us but we are not the original developers and it's hard to change them without a breaking refactor. If you are interested to join us and have enough time, I'd like to invite you as a collaborator.

Copy link
Collaborator

@jasonsaayman jasonsaayman left a comment

LGTM

@jasonsaayman jasonsaayman merged commit 7688255 into axios:master Oct 30, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chinesedfan
Copy link
Collaborator

@chinesedfan chinesedfan commented Oct 31, 2020

Bad news. CI failed in Node.js 10/14. Maybe that's the reason why it was skipped. And it's annoying that the master branch runs additional tests.

@timemachine3030
Copy link
Contributor Author

@timemachine3030 timemachine3030 commented Oct 31, 2020

Thanks for the kind words all.

Looking at the CI builds I'm guessing that there is some tear down/cleanup missing from the socket test that is causing havoc. They don't fail in the same place nor is the failure consistent between versions (I don't think that it is a version number thing, I think that it is a timing issue). I'll try to reproduce here.

@timemachine3030
Copy link
Contributor Author

@timemachine3030 timemachine3030 commented Oct 31, 2020

I was able to reproduce a failure on Windows 7: node -v: 10.0.0

  1) supports http with nodejs
       should support sockets:
     Uncaught Error: listen EACCES ./test.sock
      at Server.setupListenHandle [as _listen2] (net.js:1313:19)
      at listenInCluster (net.js:1378:12)
      at Server.listen (net.js:1477:5)
      at Context.<anonymous> (test\unit\adapters\http.js:376:8)
      at process.topLevelDomainCallback (domain.js:121:23)
@timemachine3030
Copy link
Contributor Author

@timemachine3030 timemachine3030 commented Oct 31, 2020

@chinesedfan To answer your question on collaboration: Yes. I'm game for helping out. As a mention in the review, my company uses the package. They are willing to donate some of my development time to open source software.

@chinesedfan
Copy link
Collaborator

@chinesedfan chinesedfan commented Nov 1, 2020

@timemachine3030 Good. Please send me your email address (mine is in the Github profile). Then we can invite you to our slack channel.

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

Successfully merging this pull request may close these issues.

None yet

3 participants