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

Add specs for signal exit #15229

Conversation

straight-shoota
Copy link
Member

We were missing generalized specs for Process::Status behaviour on a signal exit, particularly for #exit_code.
Predicate methods like #normal_exit? already have platform-specific tests, but this change enhances the portable portion of the spec for basic behaviour.

The behaviour of #exit_code might change in the future (see #15228), but this encodes the current status.

@straight-shoota
Copy link
Member Author

straight-shoota commented Nov 26, 2024

It seems normal_exit? and signal_exit? are entirely broken on Windows.
Darn, Windows doesn't have signals of course. So normal_exit? is true for every status, but it doesn't necessarily corresponds to ExitReason::Normal 🤦

  1) Process::Status #normal_exit?
     Failure/Error: status_for(:interrupted).normal_exit?.should be_false

       Expected: false
            got: true

     # spec\std\process\status_spec.cr:50

  2) Process::Status #exit_code
     Failure/Error: status_for(:interrupted).exit_code.should eq 0

       Expected: 0
            got: -1073741510

     # spec\std\process\status_spec.cr:30

  3) Process::Status #signal_exit?
     Failure/Error: status_for(:interrupted).signal_exit?.should be_true

       Expected: true
            got: false

     # spec\std\process\status_spec.cr:60

@straight-shoota straight-shoota added this to the 1.15.0 milestone Nov 28, 2024
@straight-shoota straight-shoota merged commit 1b02592 into crystal-lang:master Dec 1, 2024
69 checks passed
@straight-shoota straight-shoota deleted the spec/process_status-signal-exit branch December 1, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants