node-coveralls icon indicating copy to clipboard operation
node-coveralls copied to clipboard

Published 20 hours ago • verified publisher icon nickmerwin

Remove request library

Open jaeh opened this issue 5 years ago • 12 comments

please do not merge this without testing,

i have not yet tested this in any of my libraries, just made the unit tests pass, will do so in the course of today then repost here.

there also are some function calls and returns that could be made simpler, i tried to keep the changes minimal for now.

this would close #204, #267 and likely others i did not see.

jaeh avatar Feb 13 '20 20:02 jaeh

this does not work yet, throws 400, something about the content-type i guess, looking at it.

jaeh avatar Feb 13 '20 22:02 jaeh

could someone with deeper insight in the coveralls api please have a look at this?

in my tests it returned 422: failed to parse package.json.

i have a deadline on the 22.2. that i will likely miss, so i can not really investigate this earlier than that.

thanks.

jaeh avatar Feb 16 '20 03:02 jaeh

Hey, I'd like to see this fixed as soon as possible, since request's deprecation just starting causing tap to raise deprecation notices at install time, because of this dependency.

Added some comments to where I think the patch is going wrong. I'd recommend adding some tests around it, it's pretty ironic for this of all modules to not have full test coverage :)

isaacs avatar Mar 08 '20 22:03 isaacs

thanks! having a look at this now, looks like the obvious reason.

jaeh avatar Mar 10 '20 23:03 jaeh

i assume this confirms that the http request works:

https://coveralls.io/builds/28723334

coverage went down, the request file can be seen in the sources.

will see if i can get some tests done quickly.

jaeh avatar Mar 11 '20 00:03 jaeh

had a look at this and it's a bit more complicated than i would be comfortable to just implement.

have not worked with mocha/sinon for years, the seemingly logical choice to use would be nock as a mock for http.request.

i will revisit tomorrow, it's almost 2 in the morning here anyways.

jaeh avatar Mar 11 '20 00:03 jaeh

added nock to dev dependencies, this makes the test on node 6 break, maybe using sinon to mock the requests would be a better idea.

added a few tests for the request file, only the happy path and some obvious errors that now get caught before even sending the request (url or data empty or not strings)

made sure the body chunks are buffers, Buffer.concat and nock did not play well together, because nock seems to send strings.

jaeh avatar Mar 14 '20 21:03 jaeh

@jaeh can you rebase, squash and remove any unrelated changes? You have a change in test/getOptions.js that is causing tests to fail. Also you have updated all devDependencies which is also causing tests to fail.

As for nock and Node.js, can't you use an older nock version?

XhmikosR avatar Apr 24 '20 05:04 XhmikosR

NVM, I rebased it and pushed it on my fork https://github.com/nickmerwin/node-coveralls/compare/master...XhmikosR:remove-request

Only issue left is Node.js 6.x compatibility.

@nickmerwin how about dropping support for Node.js 6.x and even 8.x so that we can move on and make the new release a major version bump?

XhmikosR avatar Apr 24 '20 05:04 XhmikosR

I tried nock 10.x without any luck. Maybe we just look into using another library to replace request and keep it simple?

XhmikosR avatar Apr 24 '20 06:04 XhmikosR

Anyone looking to fix this issue, I released a fork of this library called coveralls-next as we were not getting the support we needed on this repo. https://github.com/jtwebman/coveralls-next

jtwebman avatar Jan 18 '21 20:01 jtwebman

Since this library doesn't seem to be supported anymore I fix a bunch of things on a fork if you want to check it out and are still pulling the library into your packages: https://github.com/jtwebman/coveralls-next IE no more request library and no security issues.

jtwebman avatar Mar 06 '22 22:03 jtwebman