DeeDeeG
commented
1 year ago This comment ended up being long also, bold stuff is for skimming.
While I'm pretty confident this PR works as it should, I would note in particular that the code style of the new JS script I wrote for this PR was not linted. I don't really have any strong opinions on it, but would be willing to accept any wisdom/suggestions for changes.
For example: I would be willing to run prettier or some other linter on it, to make it neater and enforce some limit to the line lengths by indenting some stuff onto the next line. Or whatever the more JS-experienced folks here might want. But I have tested on my local machine, and in CI, and it does what it's supposed to do so far.
(Codacy seems pretty unhappy about it, but a lot of that is it doesn't like single quotes ' and prefers double quotes ". And I didn't put semicolons at the ends of lines in a bunch of places. And there are some rather long lines that it wants to indent to the next line, which is understandable IMO. On closer look, it appears to be running prettier. So that would be easy enough to fix.)
(Note: You can do node script/ensure-node-source-fixture-files.js --verbose to get a lot of verbose logging output, for troubleshooting or if you're curious what the file is doing. Although the multiple file downloads happen async in parallel, so certain messages post-download for each file will be interspersed and print somewhat jumbled/out of order. So it might be a bit hard to read.)
I put a lot of comments in the new script file, so I hope it is easy to quickly get a handle of what the new script is doing.
Which reminds me, one more thing. The script I wrote has a kind of ridiculously long and verbose filename. I would be open to shortening that filename if people want it to not be quite so long.
confused-Techie
Spiker985
As per usual, I'm writing really long stuff in these PR bodies.
The short story is, this fixes the specs.
See the diff and commit messages for details. (Although I wrote some really long commit messages, too. Whoops.)
There is the summary below, which was meant to be the condensed info, but ended up being long again.
I also bolded the bare minimum info in the paragraphs below, so one can skim to know what this PR is about/why.
Summary:
script/ensure-node-source-fixture-files.js) to fetch the files from NodeJS.org on the web as-needed, (or verify the local copies if you already fetched them once) rather than check them into the repo..gitignore, since the whole point is we do not want these huge ~70MB+ files checked into git under any circumstances. They are to be fetched from the web as-needed only.npm run testscript to run the newensure-fixturesscript (which I wrote for this PR) before running the specs, so the files will (or at least should) exist locally on disk, and the specs that need these files can pass.Hi folks.
When bumping to bundled NodeJS to v16.0.0 (https://github.com/pulsar-edit/ppm/pull/41 --> https://github.com/pulsar-edit/ppm/pull/41/commits/12ae15dfd226d38e10830f0c30b65059fe76ebff), it turned out ppm stopped being able to pass certain specs.
On closer look, these specs failing in this particular way seemed very familiar to me, reminding me of the time I had to bump the copy of NodeJS's source and headers that we use to build some basic example/test modules against in certain tests. I had to do this to get specs passing again during the upgrade to node-gyp v5.X (https://github.com/atom/apm/pull/887 --> https://github.com/atom/apm/pull/887/commits/77227d829386dfb26d83fe36fc306c769d812c80)
However, the same solution again (bumping to newer NodeJS source/headers for the specs) would baloon this git repo's size by just over 100 megabytes. (Newer NodeJS versions keep getting larger and larger and larger as they continue adding new features to NodeJS core.)
So I put together a script to download these large files off the web from NodeJS.org as-needed and just keep them around for subsequent test runs, since they won't be changing often. I then made sure this script is run (and the source/header files verified or re-downloaded as needed) before each test run -- I added it to the steps that get run when you do
./bin/npm run test. I also reconfigured some spec files to expect these files to be in the newspec/fixtures/node-sourcedir where these files will live. And I.gitignored the new files, since they are not meant to be checked in to git. And I added a step to the CI workflow to cache these files, so we can somewhat reliably have these files, even if NodeJS.org flakes or goes down. This also saves NodeJS.org some bandwidth, and makes the tests run ever so slightly faster. (Although it's only faster by a second or two! None of this file downloading/verifying takes very long at all, nor does the caching in CI! I am impressed that it only takes about 5 seconds or less either way. Usually less than 5 seconds!)In other words, I fixed the specs, and made it easier to change the version of NodeJS these test/example modules build against during the specs, and did so without causing this git repo to become really super huge to download.
(Most of this repo's download size is from the NodeJS 10.20.1 source/headers I added in https://github.com/atom/apm/pull/887! I have always regretted that fact after I realized there might have been a better way to do it. So here is my shot to redeem the situation!)
(Historical note: NodeJS used to be so small that checking its entire source into git was a fairly reasonable thing to do. For version v0.10.3, all NodeJS files needed for these specs were under 1 megabyte! For NodeJS v10.20.1, one file at 0.5MB, two files just over 6.5 MB each and one file at 45MB (the NodeJS main source tarball) were needed. So that's almost exactly the size to download this repo, and it's all those NodeJS source files I added in that PR some time ago. Ah, well.)