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

test runner: cleanup on Ctrl+C/SIGINT #54

Merged
merged 5 commits into from
Nov 1, 2023
Merged

Conversation

laniakea64
Copy link
Collaborator

@laniakea64 laniakea64 commented Oct 28, 2023

Attempt to fix #45

I took another look into this, and this time something clicked about how to use ctrlc crate (which BTW just also uses), so decided to give this a try. Still don't really know what I'm doing in Rust, so @NoahTheDuke your code review would be much welcome, thanks 🙂

About test runner performance and this PR:

  • I have no idea why Vim's stderr being piped is such a performance improvement over null?? 👀 This doesn't seem to be the case for stdin or stdout.
  • Performance is more varied with this PR. Most runs are about the same performance as before, but I'm seeing variation up to ~0.03 seconds difference either way, with the faster side occurring more often than the slower side. I wasn't able to completely avoid having any "notably" (as in meaningful timing measurement) slower runs.
  • Not sure to what extent the optimizations ended up being tuned for my specific dev environment rather than more generally? 😨
  • Cargo deps were not updated again as part of this PR, because doing so this time actually reduced performance. There is a cargo-audit warning that we are using a yanked crate (ahash version 0.8.3, reasoning for the yank explained on their wiki), so the question of updating Cargo deps again should be investigated, but let's address that separately.

Copy link
Owner

@NoahTheDuke NoahTheDuke left a comment

Choose a reason for hiding this comment

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

these changes look good to me!

@laniakea64 laniakea64 merged commit f25560b into main Nov 1, 2023
1 check passed
@laniakea64 laniakea64 deleted the test-runner-cleanup branch November 1, 2023 15:07
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.

Test runner cleanup on SIGINT
2 participants