Skip to content

Don't change Encoding.default_internal in assert_raise_with_message #14210

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 15, 2025
Merged

Don't change Encoding.default_internal in assert_raise_with_message #14210

merged 1 commit into from
Aug 15, 2025

Conversation

peterzhu2118
Copy link
Member

For most tests (except two), we don't need to change Encoding.default_internal in assert_raise_with_message. We're trying to run the test suite across multiple Ractors and modifying Encoding.default_internal can cause other concurrently running tests to fail.

@eregon
Copy link
Member

eregon commented Aug 13, 2025 •
edited
Loading

Note: basically a redo of c32ce5f + f356bd8. That got reverted in cf4d4c3.
BTW r54522 is 404bf57.
I don't recall why the revert of both my commits but probably some CI was failing, maybe on some exotic platform or Windows or so which has/had a non-default UTF-8 external encoding (though this is about internal encoding). Encodings have changed a bit on Windows so it might be fine now.

In any case, 👍 from me.

@launchable-app Launchable app

This comment has been minimized.

Sign in to view
@eregon
Copy link
Member

eregon commented Aug 13, 2025

I think back then I also thought it's really not great that any core #inspect depends on internal encoding, which is maybe what I meant by More general fix coming.. Not sure what happened to that, maybe I filed a redmine issue.

I wonder what's the use case for Encoding.default_internal actually, it's nil by default and I don't recall any real code setting it.

@peterzhu2118
Don't change Encoding.default_internal in assert_raise_with_message
5a1724f 
For most tests (except two), we don't need to change Encoding.default_internal
in assert_raise_with_message. We're trying to run the test suite across
multiple Ractors and modifying Encoding.default_internal can cause other
concurrently running tests to fail.
@peterzhu2118 peterzhu2118 force-pushed the assert-raise-with-msg-default-internal branch from 04d76d3 to 5a1724f Compare August 14, 2025 13:26
@peterzhu2118 peterzhu2118 merged commit 79f5202 into ruby:master Aug 15, 2025
85 checks passed
@peterzhu2118 peterzhu2118 deleted the assert-raise-with-msg-default-internal branch August 15, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peterzhu2118 @eregon