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

[Breaking] Change match_raises to be more intuitive #419

Merged
merged 2 commits into from
Mar 13, 2025

Conversation

psafont
Copy link
Contributor

@psafont psafont commented Feb 12, 2025

I don't know whether breaking behaviour is acceptable or not, opening to receive feedback.

I can always drop the commit with the breaking change and keep the one documenting current behaviour, suggestions on how to deal with this is welcome.

Fixes #418

Current behaviour is unintuitive, let users know how to use the
function, and test it
Change the user-defined function to better match the expectation to
"match" the expected exception.

Fixes mirage#418
@psafont
Copy link
Contributor Author

psafont commented Feb 12, 2025

The failures are:

  • in windows is because the native npm can't be found
  • In the lower bounds check because of a type mismatch (unrelated code to this change)
  • core_unix 0.17 not compiling with ocaml 5.3.0 (and 0.16 with 4.14 under alpine)

Copy link
Collaborator

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

I'm in favor of introducing a breaking change to fix the API. Users can set the lower-bound.

Copy link
Member

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for the fix @psafont

@psafont
Copy link
Contributor Author

psafont commented Mar 12, 2025

Just in case people are waiting on me merging this PR, I can't merge it :)

@samoht
Copy link
Member

samoht commented Mar 13, 2025

Thanks for the fix

@samoht samoht merged commit b3a4a62 into mirage:main Mar 13, 2025
10 of 12 checks passed
samoht added a commit to samoht/opam-repository that referenced this pull request Mar 13, 2025
CHANGES:

- Add `seq`, a testable for `Seq.t` and `contramap` (mirage/alcotest#412 @xvw)
- Expose the `V1.Skip` exception (mirage/alcotest#415, mirage/alcotest#416, @Khady)
- BREAKING FIX: `match_raises` now expects the user-defined function to return
  true for expected exceptions. Previously false was interpreted as an
  expected exception. (mirage/alcotest#418, mirage/alcotest#419, @psafont)
@psafont psafont deleted the exnpect branch March 13, 2025 09:54
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.

Alcotest.match_raises has unintuitive behaviour
4 participants