Skip to content

Update YAML specs for Psych 4#837

Closed
headius wants to merge 1 commit intoruby:masterfrom
headius:psych4
Closed

Update YAML specs for Psych 4#837
headius wants to merge 1 commit intoruby:masterfrom
headius:psych4

Conversation

@headius
Copy link
Contributor

@headius headius commented May 20, 2021

Psych 4 no longer passes the YAML specs. This PR will fix or reorganize the specs to support 4.0.0 behavior.

Psych 4.0 defaults `load` behavior to `safe_load` so these unsafe
loads need to be guarded on version. Safe versions should probably
be added.
@headius
Copy link
Contributor Author

headius commented May 20, 2021

First commit fixes the unsafe class loading happening in a few load specs by moving those specs to a shared unsafe_load spec. They are run as part of load specs on Psych < 4.0.0 and as part of unsafe_load specs on Psych >= 4.0.0. I did not update the specs to make them "safe" as I am mostly just trying to get them green agan.

@eregon
Copy link
Member

eregon commented May 20, 2021

Quick note: there were already changes to deal with this in https://github.com/ruby/ruby/tree/master/spec/ruby, which are not sync'd here yet.

@eregon
Copy link
Member

eregon commented May 20, 2021

Your split seems better though, thanks!
I'll try to use that when synchronizing specs (end of the month).

@headius
Copy link
Contributor Author

headius commented May 20, 2021

Would it not be best for gem-based specs to guard on the version of the library going forward? That will allow implementations more freedom to ship newer gems in update releases.

It is also not possible to run Psych 4.0 specs on any Ruby less than 3.1 if we guard on Ruby version >=3.1. This will make Psych dev more cumbersome. Perhaps the Psych specs should move under ruby/psych?

I assume the other failures have been addressed in ruby/ruby so I won't go into them here.

@eregon
Copy link
Member

eregon commented May 20, 2021

Yes, guarding on the Pysch version as you did here is definitely better.
(Although I guess shipping Psych 4 in stdlib for older Rubies than 3.1 would be impractical as it could cause a lot of incompatibility)

Moving those specs to Psych could make sense, but it would need some effort to get mspec available there, integrate in CI, and still make it easy for other Ruby implementations to run those specs in their CI.
Typically specs for stdlibs trigger implementation-specific bugs, so I see it as a subset of tests to check stdlibs are working and edge cases depending on implementation details are covered.
Probably best long term to move the specs to the respective gems when possible, but I guess I won't have the time for that anytime soon.

eregon added a commit that referenced this pull request Jun 2, 2021
* Based on #837
* Run both with YAML.load on Psych < 4, only safe on Psych >= 4
* Run both with YAML.unsafe_load (available in Psych >= 4)

Co-authored-by: Charles Oliver Nutter <headius@headius.com>
@eregon
Copy link
Member

eregon commented Jun 2, 2021

Integrated in c6e9285 (given the conflicts rebasing or merging was too hard)
Thanks for the PR!

@eregon eregon closed this Jun 2, 2021
@headius headius deleted the psych4 branch June 2, 2021 19:58
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.

2 participants