[Bug 1372718] Review Request: rubygem-puma - A simple, fast, threaded, and highly concurrent HTTP 1.1 server

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1372718



--- Comment #14 from Vít Ondruch <vondruch@xxxxxxxxxx> ---
(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@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]