-
-
Notifications
You must be signed in to change notification settings - Fork 13
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 request
Migrate to superagent
&& Fix CI
#87
Conversation
We really need to figure out our testing situation. I was initially excited seeing green CI, but on closer inspection, the amount of passed tests shown as dots, does not match the amount we expect to test. Nor is there any kind of summary for how many tests have passed, like there should be. These tests are failing somewhere, and with our testing situation eating any output, there's no way to know where or how many. Most worrisome of all, this is reported as passed tests. But something is broken. |
So on this, I've done a lot of investigating what's going on with our tests. And I hate to say, I think it's a But at the very least, I've found a way locally to discover what's going on. Although a bit troublesome. To get actually logs from these failed tests, you'll need to add the following snippet to the top of the file: process.on("uncaughtException", (err) => {
fs.writeFileSync(`${__dirname}/log.txt`, JSON.stringify(err) + "\n", { encoding: "utf8", flag: "a+"});
fs.writeFileSync(`${__dirname}/log.txt`, JSON.stringify(err.stack) + "\n", { encoding: "utf8", flag: "a+"});
}); Once you run your tests, and if they are failing without any output like seen here, a Which for reference, in this current PR, here's the output: This PR Output
|
So on this one, as you can see CI was troublesome. I have found a way around the issue of the So with that in mind I've added a fix to the CI here as well. While a bit verbose, it essentially preforms the duties that are reserved for the main file of Once this is merged then we can go ahead and use these advances in CI to resolve the issues being seen by #88 |
Also an edit, the single failing windows test is not the fault of this package. And is resolved in #86 |
request
Migrate to superagent
request
Migrate to superagent
&& Fix CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't gotten around to fully reviewing src/request.js
.
Just had a few comments.
Hopefully makes the remaining review less to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests pass on my machine locally, and in CI, so I'm inclined to believe this doesn't break anything.
And i think it's really nice that src/request.js
wraps the external dependency, so we can isolate the churn to there and have the rest of the code mostly unchanged.
I would love to get a chance to fully review all of the changes in src/requests.js
, but if I never get around to that, it's at a "lite approve" 👍 from me, which is: I wish I could review it more thoroughly at the time of giving the "lite approve", but I feel somewhat responsible for the feature or change's future success, as a reviewer today, and would be very willing to assist in any troubleshooting, debugging, release management hoop jumping, etc. should there be any issue in the future.
And that I've done whatever due diligence I can do short of a full review of the code line by line to ensure this seems like it won't have show-stopper or large unintended fallouts from any issues.
I suppose I do want to mention there's nothing in the specs about proxies, at least not that I could find with a quick code search, so I hope that anyone who is familiar with using (HTTP, HTTPS) proxies can give that a manual kick in the tires, so to speak, give it a real-world test to make up for the lack of automated tests for it.
Yeah, I could live with this merging as-is. I am gearing up to try and review any more of it I can get to soon, but I don't want to delay it too much further on account of my review timeline.
P.S. this sort of assumes the review comments above are addressed or resolved. Sorry about those!
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
@DeeDeeG Thanks for taking a look at this one. I know there's a lot of changes in here. But as this change isn't really dependent on much, except maybe it's nice to have the CI changes available, overall this one can wait, so it'd be fine to wait until a proper review can occur. Otherwise if we want to merge as as, I'm also totally down for that as well. But on your notes about proxies, worrying to know there's no testing for it. I will say, I'm not an expert in that field, so my changes interacting with proxies, is mostly straight out of docs for the services we use. |
I never ended up properly reviewing this in ~2 weeks, but I would be comfortable merging as per my lite-approve/comment above. That said, I'll point out Codacy has some actually interesting suggestions here and there, if you ignore all the whitespace-only suggestions and Codacy apparently preferring double-quote marks Anything else you'd want to see done with this? Or should we go for it and merge this? (Also, sorry some recent PR's have made merge conflicts in the |
@DeeDeeG I appreciate you giving this PR some attention. Although looking at Codacy, I'm personally not seeing anything super motivating unless I've just missed it. Really seems we should probably get a codacy config file in this repo to start ignoring some of these pointless rules and warnings it's trying to enforce. Beyond that, other than fixing the merge conflict I'm still confident in this PR, if we think it's good to go. |
@confused-Techie the "var defined but never used" stuff seemed the most possibly interesting unless it was misinterpreting the point of those vars. (Ctrl + F "never used" on the Codacy, since there's so much whitespace and quote mark suggestions to ignore.) And there's one about But I honestly leave it to your discretion, since I should've brought these up sooner in review if they were important, this has been delayed quite a bit already, so it doesn't seem super fair to hold it up on tiny things like this. |
@DeeDeeG I've gone ahead and addressed really only one of the warnings. The main reason being, is a few of them are coming from the And I'd really rather not mess with that in unknown ways tbh. So if we aren't too concerned about it, may be best to leave it alone. But otherwise, I've gone ahead and resolved the conflicts, and updated the But no worries about this one taking some time, it's not the most urgent PR. But hopefully we are good to go now, I really do appreciate you taking a look! |
One of those "weird mysteries of CI". For only the NodeJS 18.x runs... The
And it does install So, there is no ????????????????????????? This doesn't happen for me locally. I have no idea what is going on with WTF GitHub Actions? I don't even. |
@DeeDeeG Yeah this makes no sense. Unless it's some issue with the changes to But I would've expected we would see this on that original PR. But tried rerunning, ensured we don't have any type of cache for our runners, but seems to consistently fail here. But like you, can't get it to happen locally. Although, to be fair I'm not on Node 18 locally. So may need to install that and try again, but how weird. |
Yeah, it happens on FWIW, some Node+npm versions changed since 5 days ago when Before (working):
After (buggy):
I installed this version of node (v18.18.0) and npm (9.8.1) locally on macOS, and everything works fine. I seriously don't get it. But the issue can reproduce on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a "rubber stamp" Approve, since I didn't manage to review all of request.js
and the jasmine changes in-detail, though I vaguely remember some time long ago I did confirm that the changes to jasmine are actually quite minimal... I think I did, anyway.
This PR has been open for quite a while and doing well in the repo's automated tests, so I'll give it an Approve and try to keep up with anything that might go wrong, for triage and issue resolution purposes.
CI failure is unrelated to this PR. It can be seen affecting a deliberate re-run of master
branch as well. (https://github.com/pulsar-edit/ppm/actions/runs/6210843978) (I have no idea what caused this, other than it looks like @actions/setup-node
's fault, maybe, and I hope it goes away on its own, since we didn't cause it. FWIW I have seen similar transient CI things like this before, I thin specifically with different @actions/setup-node
version updates, and it just went away on its own those times. Weird stuff, but not our fault.)
Thanks for the efforts! Maybe this can even (incidentally) make debugging the test problems/errors on Windows a little less cryptic... Hopefully! As in, maybe we can actually see the output from the test suite on Windows now??? Worth checking... (Long-term goal: Get everything passing on Windows!)
Long way of saying "Approve".
@DeeDeeG Thanks a ton for taking such a good look at this, and especially confirming this failure isn't the fault of this PR. Suppose in the short term we could always disable testing on the newer node version, as it's meant really to warn us of things breaking, since we don't actually use anything other than 16 within Pulsar right now. Although, this does maybe indicate something breaking, but I'm very much hoping you are right, in that this will just go away on it's own. But I'm very much on board with using these changes to see if we can finally enable, and fix Windows tests, as that would be quite awesome! But with your approval, I'll happily merge this one, and hopefully soon take a look at running some Windows tests! |
This PR intends to fully remove
request
from the codebase, as it's deprecated, and no longer receiving updates.This starts by removing
request
from./src/request.js
which is the file all others use to preform any network requests, afterwards hopefully removingrequest
from the downloading node scripts should prove much easier. But as tests can't run on Windows yet, I'll have to rely on CI to determine success in these changes, other than manual testing.