Migrate pathname.c to pathname_builtin.rb#14303
Conversation
This comment has been minimized.
This comment has been minimized.
```
Error: test_join(PathnameInstanceTest): NoMethodError: undefined method 'respond_to?' for an instance of RBS::UnitTest::Convertibles::ToStr
/Users/hsbt/Documents/github.com/ruby/ruby/gems/src/rbs/lib/rbs/unit_test/spy.rb:94:in 'Pathname#join'
/Users/hsbt/Documents/github.com/ruby/ruby/gems/src/rbs/lib/rbs/unit_test/spy.rb:94:in 'block (2 levels) in wrapped_object'
/Users/hsbt/Documents/github.com/ruby/ruby/gems/src/rbs/lib/rbs/unit_test/type_assertions.rb:152:in 'block in RBS::UnitTest::TypeAssertions#send_setup'
/Users/hsbt/Documents/github.com/ruby/ruby/gems/src/rbs/lib/rbs/unit_test/type_assertions.rb:150:in 'Kernel#catch'
/Users/hsbt/Documents/github.com/ruby/ruby/gems/src/rbs/lib/rbs/unit_test/type_assertions.rb:150:in 'RBS::UnitTest::TypeAssertions#send_setup'
/Users/hsbt/Documents/github.com/ruby/ruby/gems/src/rbs/lib/rbs/unit_test/type_assertions.rb:172:in 'RBS::UnitTest::TypeAssertions#assert_send_type'
test/stdlib/Pathname_test.rb:389:in 'PathnameInstanceTest#test_join'
386: Pathname('.'), :join, 'foo'
387: assert_send_type '(String, String) -> Pathname',
388: Pathname('.'), :join, 'foo', 'bar'
=> 389: assert_send_type '(ToStr) -> Pathname',
390: Pathname('.'), :join, ToStr.new('foo')
391: assert_send_type '(Pathname) -> Pathname',
392: Pathname('.'), :join, Pathname('foo')
```
|
I could reproduce the failures of |
|
Thank you for this, regarding the extra commits I think it would be best to have all/some of them in ruby/pathname too.
|
| # Windows platforms may allocate an extra array after https://github.com/ruby/ruby/pull/14303 | ||
| unless ($allocations[0] - num_arrays == 1 && RUBY_PLATFORM =~ /mswin|mingw/) | ||
| failures << "Expected \#{num_arrays} array allocations for \#{check_code.inspect}, but \#{$allocations[0]} arrays allocated" | ||
| end |
There was a problem hiding this comment.
This is really surprising, how is this possible given this file doesn't seem to use Pathname at all?
There was a problem hiding this comment.
Ah probably because of #14303 (comment). I don't quite understand how having a bigger pathname_builtin.rb can cause that though.
There was a problem hiding this comment.
Because I requested to separate the original PR of ruby/pathname#57 (comment).
But you ignored my suggestions and stubbornly refuse to accept them. As a result, Windows platform is broken and I waste a many of investigation time.
There was a problem hiding this comment.
Because I requested to separate the original PR of ruby/pathname#57 (comment).
As far as I can tell smaller PRs would not have helped or changed anything about that.
This PR revealed that ruby/pathname did not test against ruby/spec, which I fixed in ruby/pathname#60.
I also found significant bugs in test_pathname.rb which I fixed in that PR too.
But you ignored my suggestions and stubbornly refuse to accept them.
I asked you to make concrete suggestions on the PR in ruby/pathname#57 (comment), you did not in one month, so I couldn't understand what you wanted.
I also explained why I couldn't see how to meaningfully make smaller PRs for that change.
I did not understand precisely what you meant by #13906 since that seemed unrelated to Pathname as far as I could tell.
It turns out a related issue happened on this PR, but that is clearly a bug of ObjectSpace.count_objects, not of Pathname. BTW I tried to fix that bug #14329 to help move things forward.
Excluding the test like you did here seems fine to me, it was a bug unrelated to Pathname, basically a transient test failure waiting to happen (it even already failed before I guess as the reason for #13906) due to a bug in ObjectSpace.count_objects.
As a result, Windows platform is broken
No CI was broken. Only failures on this PR when updating to latest ruby/pathname.
As far as I understand there was no urgency to merge this PR or make it green.
and I waste a many of investigation time.
I am sorry about that, I sympathize with that feeling, it's always annoying when unexpected failures happen.
I tried to react here and on https://bugs.ruby-lang.org/issues/21532 as fast as possible and offer help.
If such a situation would happen again, please ask help and let others help, rather than spending too much of your time on it and getting frustrated.
I would also like to help with the sync of ruby/pathname <-> ruby/ruby in future, if you would like so.
|
I would like to help with "upstreaming" these commits to ruby/pathname this weekend, if that's OK. |
|
Looking at https://github.com/ruby/ruby/actions/runs/17144600559/job/48638223862 (failures on the first commit) that's what showed the differences, so ruby/spec has extra tests that test_pathname.rb is missing. We should either run that part of ruby/spec in ruby/pathname, or port those specs to test_pathname.rb. |
|
All commits except Skip rbs tests for pathname_bundle.rb and Omit extra allocation of Array object in Windows platform temporary upstreamed in ruby/pathname#60 |
Import from ruby/pathname#57