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

fix test regexes for musl libc #2917

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Jan 4, 2024

Error messages are slightly different when using musl libc.

The new regexes fix that.

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e317d9c) 99.67% compared to head (282c00c) 99.67%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2917   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files         115      115           
  Lines       17308    17308           
  Branches     3109     3109           
=======================================
  Hits        17252    17252           
  Misses         38       38           
  Partials       18       18           
Files Coverage Δ
src/trio/_tests/test_fakenet.py 100.00% <100.00%> (ø)
...c/trio/_tests/test_highlevel_open_tcp_listeners.py 100.00% <ø> (ø)

@A5rocks
Copy link
Contributor

A5rocks commented Jan 4, 2024

Where did you run into this? (So we can add that to CI!)

@Fuyukai
Copy link
Member

Fuyukai commented Jan 4, 2024

WHY is this using regexes. OSError literally has an errno property. Against this with vehemence. Somebody fix the tests properly.

@CoolCat467
Copy link
Member

WHY is this using regexes. OSError literally has an errno property. Against this with vehemence. Somebody fix the tests properly.

It's using regexes because pytest.expect's match argument is a regular expression, not literal string compare. This was a part of my work enabling ruff's flake8-pytest-style rule in #2822 so we can be even more precise with what errors we expect to happen in the test suite.

Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this, we should really add CI testing for this platform!

@jakkdl
Copy link
Member

jakkdl commented Jan 5, 2024

WHY is this using regexes. OSError literally has an errno property. Against this with vehemence. Somebody fix the tests properly.

Disapprove of the tone.

And scrolling through the file it looks like pretty much all tests does check the errno property. Checking the string in addition is maybe overkill, but I like the check in most all other instances - and it's doing what it's supposed to by letting us know about a new system that behaves differently.

@jakkdl
Copy link
Member

jakkdl commented Jan 5, 2024

Where did you run into this? (So we can add that to CI!)

some quick searching for "musl libc":
https://musl.libc.org/
https://peps.python.org/pep-0656/
FRRouting/frr#8442

So seems like the easiest way is to add Alpine Linux to the CI. Perhaps something like https://github.com/marketplace/actions/setup-alpine-linux-environment

@tornaria
Copy link
Contributor Author

tornaria commented Jan 5, 2024

Where did you run into this? (So we can add that to CI!)

Upgrading python3-trio package for void linux (which supports both glibc and musl) in void-linux/void-packages#47946.

Since void linux is rolling release, alpine is probably easiest to implement.

@A5rocks
Copy link
Contributor

A5rocks commented Jan 5, 2024

Yeah I was confused cause we should be running Alpine but then I remembered our sr.ht CI stuff stopped working...

I agree that it's somewhat strange that we check the strings via regex rather than check errnos but this is also tests and tbh being more strict is fine. (I note that the 2nd and 3rd error doesn't have one specific errno... and IMO it's easier to look at a test and see "Socket not connected" than see "107" and have to do another lookup or potentially have an out-of-date comment).

@A5rocks A5rocks merged commit f673cfd into python-trio:master Jan 5, 2024
29 checks passed
@trio-bot
Copy link

trio-bot bot commented Jan 5, 2024

Hey @tornaria, it looks like that was the first time we merged one of your PRs! Thanks so much! 🎉 🎂

If you want to keep contributing, we'd love to have you. So, I just sent you an invitation to join the python-trio organization on Github! If you accept, then here's what will happen:

  • Github will automatically subscribe you to notifications on all our repositories. (But you can unsubscribe again if you don't want the spam.)

  • You'll be able to help us manage issues (add labels, close them, etc.)

  • You'll be able to review and merge other people's pull requests

  • You'll get a [member] badge next to your name when participating in the Trio repos, and you'll have the option of adding your name to our member's page and putting our icon on your Github profile (details)

If you want to read more, here's the relevant section in our contributing guide.

Alternatively, you're free to decline or ignore the invitation. You'll still be able to contribute as much or as little as you like, and I won't hassle you about joining again. But if you ever change your mind, just let us know and we'll send another invitation. We'd love to have you, but more importantly we want you to do whatever's best for you.

If you have any questions, well... I am just a humble Python script, so I probably can't help. But please do post a comment here, or in our chat, or on our forum, whatever's easiest, and someone will help you out!

@tornaria tornaria deleted the test-musl branch January 6, 2024 18:54
@jakkdl jakkdl mentioned this pull request Jan 7, 2024
17 tasks
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.

5 participants