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

Replace shell scripts #325

Merged
merged 16 commits into from
Dec 7, 2024
Merged

Conversation

lishaduck
Copy link
Contributor

@lishaduck lishaduck commented Dec 4, 2024

Begin working toward the goals in #176.

VERY WIP!!!

After all that work trying to fix the shell, I decided to just nuke it and get copilot to do a basic js rewrite, which exposed a bunch of bugs. This simplifies the code at a (likely) small speed decrease.


Eventually (not in this PR 🤞), I'd like to replace these with test cases in Jest; even more eventually, I'd like to quit spawning processes in Jest tests, which would allow us to mock the filesystem using memfs and go back to running stuff in parallel (and give a massive perf boost, tests have a lot of overhead right now).

@lishaduck lishaduck changed the title Kill Shell ~~Kill Shell~~ Replace shell scripts Dec 4, 2024
@lishaduck lishaduck changed the title ~~Kill Shell~~ Replace shell scripts Replace shell scripts Dec 4, 2024
@lishaduck lishaduck marked this pull request as draft December 4, 2024 16:07
@lishaduck lishaduck force-pushed the no-more-shell-tests branch from d9021f3 to fbf71c5 Compare December 5, 2024 05:54
Copy link

socket-security bot commented Dec 5, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/zx@8.2.4 environment, filesystem, network, shell, unsafe +4 3.26 MB google-wombot

View full report↗︎

Copy link
Contributor Author

@lishaduck lishaduck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something broke in the rebase, but I'm otherwise satisfied with this iteration, and it seems to fix CI. (Once I fix it the rebase, can you retrigger ci a few more times to double check, @jfmengels?)

@lishaduck lishaduck marked this pull request as ready for review December 5, 2024 05:58
@lishaduck
Copy link
Contributor Author

lishaduck commented Dec 5, 2024

@jfmengels, ping :)

(Oh, and can we squash merge this?)

@lishaduck lishaduck mentioned this pull request Dec 5, 2024
jfmengels and others added 16 commits December 7, 2024 18:07
My IDE runs this on save, so I'm splitting out these changes.
But only run them in CI for now.
zx is a much simpler way to run commands.
It's a much higher level library than child_process,
so the code is much simpler to read.
Length-wise, it's about the same, but slightly longer,
primarily because Prettier wraps lists but not strings.
I also converted the remaining bits of sync io to async io,
as async io is only faster the more you use it.
It also encourages function coloring,
which makes it more explicit where there's io.
(Yes, I know function coloring is generally a bad thing.)
@jfmengels jfmengels force-pushed the no-more-shell-tests branch from 8f008ad to 0e582c8 Compare December 7, 2024 17:08
@jfmengels jfmengels merged commit 7520c6b into jfmengels:main Dec 7, 2024
3 checks passed
@jfmengels
Copy link
Owner

This is a great step forward! Thank you @lishaduck 🎉

@jfmengels
Copy link
Owner

@lishaduck I think the test-run-record might be broken, as some snapshots now get deleted. Maybe it's related to GitHub rate limits (but it didn't cause files to be removed). I'll try again later tonight if I get the time.

@lishaduck
Copy link
Contributor Author

lishaduck commented Dec 7, 2024

@lishaduck I think the test-run-record might be broken, as some snapshots now get deleted. Maybe it's related to GitHub rate limits (but it didn't cause files to be removed). I'll try again later tonight if I get the time.

Hm. Oh, yep. Added this to stop it from destroying my standing with gh while testing:

if (!REMOTE && !SUBCOMMAND && (!CI || !AUTH_GITHUB)) {
process.exit(0);
}

It should be (!SUBCOMMAND || (...))

EDIT: No... hrg, logic. (If REMOTE is on or you're running record), turn it off (don't exit, keep running). ((If you're not in ci and are unauthenticated), turn it on (exit, don't keep running), but only if (REMOTE is off and you're not running record)).

@lishaduck lishaduck mentioned this pull request Dec 7, 2024
@lishaduck lishaduck deleted the no-more-shell-tests branch December 8, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants