https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #14 from Vít Ondruch <vondr...@redhat.com> --- (In reply to Jun Aruga from comment #11) > (In reply to Vít Ondruch from comment #6) > > * Purpose of Patch1? > > - Not sure about purpose of the Patch1, since if I comment it out and use > > the previous commands, I can execute the test suite just fine => I > > suggest > > to drop this patch altogether. > > - You are right that upstream is inconsistent in this regard, but I'd say > > that the particular require you are fixing is the way how the test > > suites > > typically works, while the proposed fix is antipattern. IOW you should > > propose fix fixing all the other files. > > I thinks the Patch1 is needed. > Because I got an error without the Patch1. It depends on the order of test. > > <mock-chroot> sh-4.3# > RUBYOPT=-I.:lib:/builddir/build/BUILD/rubygem-puma-3.6.0/usr/lib64/gems/ruby/ > puma-3.6.0 \ > > ruby -r minitest/autorun -e 'Dir.glob "./test/test_http11.rb", > > &method(:require)' -- -v > /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require': > cannot load such file -- testhelp (LoadError) > from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in > `require' > from > /builddir/build/BUILD/rubygem-puma-3.6.0/usr/share/gems/gems/puma-3.6.0/test/ > test_http11.rb:4:in `<top (required)>' > from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in > `require' > from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in > `require' > from -e:1:in `glob' > from -e:1:in `<main>' > > > > When I add "-r test/testhelp", its test was passed. > > <mock-chroot> sh-4.3# > RUBYOPT=-I.:lib:/builddir/build/BUILD/rubygem-puma-3.6.0/usr/lib64/gems/ruby/ > puma-3.6.0 \ > > ruby -r minitest/autorun -r test/testhelp -e 'Dir.glob > > "./test/test_http11.rb", &method(:require)' -- -v > ... > 100% passed Of course you are right in this case, but the scope of the issue is much broader then you think. Please check these lines: https://github.com/puma/puma/blob/master/test/testhelp.rb#L5-L8 You can see that the "." and "./test" gets on the load path at some point. So you can then require either "testhelp" or "test/testhelp". The correct fix would be to unify this, i.e. you should be able to do only "require 'testhelp'" at the end, because this is how it is typically done nowadays. The "." should not get on the load path at all. IOW, you can keep it like this for the moment. But I'd suggest you to work with upstream to make this consistent and up to date. This should be the start of the upstream work: ``` find test -name '*.rb' -exec sed -i "s|test/testhelp|testhelp|" {} \; ``` Otherwise the changes looks reasonable. Thanks. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
