Move remaining core specs to use fewer shared examples#1369
Conversation
f311e61 to
f51cf1d
Compare
|
Let me split this up in two PRs then |
f51cf1d to
304b83a
Compare
304b83a to
83885f1
Compare
This shared example didn't even care about the method. So I just mentioned in one of the methods that tests are found in a different file
83885f1 to
a62129f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 150 out of 150 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
core/io/tty_spec.rb:1
- This spec can be flaky across CI/container environments because
/dev/ttymay be missing or inaccessible (e.g.,Errno::ENOENT,Errno::EACCES,Errno::ENODEV), not justErrno::ENXIO. Consider broadening the rescued errors (or guarding by checking existence/permissions) so the spec reliably skips when TTY access is not available.
core/file/to_path_spec.rb:1 - This
platform_is/guardblock definesbefore/afterhooks but contains no examples that use@dir. Either add the missing TMPFILE-related examples within this scope or remove the unused hooks to avoid dead setup code.
core/float/fdiv_spec.rb:1 - The second example’s description duplicates the first one verbatim, which makes failures harder to interpret. Rename the second description to reflect the distinct intent (e.g., division by a very large Integer).
core/kernel/dup_spec.rb:1 - The spec that verified
#dupcalls#initialize_copywas removed, which drops coverage for an important and observable part of Ruby’s object-copying semantics. Consider re-adding an explicit example assertinginitialize_copyis invoked (or adding equivalent coverage elsewhere in the same spec suite) so regressions induphooks are caught.
core/kernel/clone_spec.rb:1 - Similarly to
dup_spec, removing theinitialize_copyexpectation for#clonereduces coverage for clone’s copy hook behavior. Re-introduce an example that assertsinitialize_copyis called duringcloneto keep coverage of this user-visible contract.
eregon
left a comment
There was a problem hiding this comment.
Great stuff and some of the deduplication notably for Marshal will be very nice when improving those specs.
| describe "ENV.merge!" do | ||
| it_behaves_like :env_update, :merge! | ||
| it "is an alias of ENV.update" do | ||
| ENV.method(:merge!).should == ENV.method(:update) |
There was a problem hiding this comment.
Minor: I think merge! is more common than update (for Hash and ENV)
| describe "File.delete" do | ||
| it_behaves_like :file_unlink, :delete | ||
| it "is an alias of File.unlink" do | ||
| File.method(:delete).should == File.method(:unlink) |
There was a problem hiding this comment.
Minor: I think File.delete is more common than File.unlink
| it "calls #initialize_copy on the new instance" do | ||
| clone = @obj.clone | ||
| ScratchPad.recorded.should_not == @obj.object_id | ||
| ScratchPad.recorded.should == clone.object_id | ||
| end |
There was a problem hiding this comment.
Removed because it's a bad spec and in fact clone calls initialize_clone, which calls initialize_copy?
Also there are specs that initialize_clone is called below, so this is fine.
| it "calls #initialize_copy on the new instance" do | ||
| dup = @obj.dup | ||
| ScratchPad.recorded.should_not == @obj.object_id | ||
| ScratchPad.recorded.should == dup.object_id | ||
| end |
There was a problem hiding this comment.
Removed because it's a bad spec and in fact clone calls initialize_dup, which calls initialize_copy?
Seems there is already another spec checking for initialize_copy but not for initialize_dup, but well that's out of scope.
I wonder why so many of these specs use custom objects instead of mocks though, seems unnecessarily obscure. Maybe because MSpec didn't have mocks back then (just a guess)?
|
|
||
| describe "MatchData#==" do | ||
| it_behaves_like :matchdata_eql, :== | ||
| it "is an alias of MatchData#eql?" do |
There was a problem hiding this comment.
Minor: == is more frequent than eql? (and eql? for core types is is separate than == for probably only numeric types and maybe 1-2 other classes)
| describe "Method#==" do | ||
| it_behaves_like :method_equal, :== | ||
| it "is an alias of Method#eql?" do | ||
| Method.instance_method(:==).should == Method.instance_method(:eql?) |
There was a problem hiding this comment.
Same, == is the main one I'd say and eql? the alias
| it_behaves_like :method_to_s, :to_s | ||
| it_behaves_like :method_to_s_aliased, :to_s, -> meth { meth } | ||
| it "is an alias of Method#inspect" do | ||
| Method.instance_method(:to_s).should == Method.instance_method(:inspect) |
There was a problem hiding this comment.
For Float we have inspect is alias of to_s, I think that's good because to_s is more frequent than inspect, could you follow the pattern used for Float for other classes then?
It's quite chonky but mostly more of the same for #1364. This converts the entirety of core specs.
Before:
3805 files, 35698 examples, 273269 expectations, 0 failure, 0 errors, 0 tagged
After:
3805 files, 34586 examples, 256944 expectations, 0 failure, 0 errors, 0 tagged
1112 fewer specs.