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 to cover the fix for [Bug #14266] #858

Merged
merged 3 commits into from
Oct 16, 2021

Conversation

wenderjean
Copy link
Contributor

Covering "Set#clone(freeze: false) makes frozen internal hash" from #823

Kernel#clone when called with the freeze: false keyword will call
#initialize_clone with the freeze: false keyword.
[Bug #14266]

@eregon
Copy link
Member

eregon commented Oct 7, 2021

Thank you for the contribution.
The spec should also include the behavior change though, not only the specific bug with Set (which is still useful to test though).

I think it should be a spec of Kernel#clone, in core/kernel/clone_spec.rb, and should test that initialize_clone(freeze: false) is called by clone(freeze: false).

library/set/clone_spec.rb Outdated Show resolved Hide resolved
@wenderjean
Copy link
Contributor Author

@eregon I wonder if we should remove that library/set/clone_spec.rb and validate the behavior only in core/kernel/clone_spec.rb as you suggested? One other question, when you say should test that initialize_clone(freeze: false) is called by clone(freeze: false), I should do it by using kind of a spy (ensure it was called once with) or just testing that the new copy is not frozen as I did with the current expectation?

@eregon
Copy link
Member

eregon commented Oct 7, 2021

@eregon I wonder if we should remove that library/set/clone_spec.rb and validate the behavior only in core/kernel/clone_spec.rb as you suggested?

Might be better since there is no Set#clone method.
We could still test the specific case of Set in core/kernel/clone_spec.rb to ensure that works as it was found to be potentially problematic.

One other question, when you say should test that initialize_clone(freeze: false) is called by clone(freeze: false), I should do it by using kind of a spy (ensure it was called once with) or just testing that the new copy is not frozen as I did with the current expectation?

A kind of spy, you can use obj.should_receive(method).with(args), see the existing specs doing that.

@eregon
Copy link
Member

eregon commented Oct 15, 2021

@wenderjean Could you add specs in core/kernel/clone_spec.rb as discussed above?

@wenderjean
Copy link
Contributor Author

@eregon Sure, I didn't push my changes yet because I'm struggling to configure a spy to ensure initialize_clone is called, the other examples in the repository are different from what I need. I'll push what I have so far.

@eregon eregon force-pushed the chore/add-test-for-set-clone-bugfix branch from 6951ee5 to a167809 Compare October 16, 2021 11:41
@eregon eregon merged commit 6af6aa6 into ruby:master Oct 16, 2021
@eregon
Copy link
Member

eregon commented Oct 16, 2021

Thanks, I ended using a fixture class, see the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants