-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Ignore default_path
test cases for Ruby with custom configuration
#7187
base: master
Are you sure you want to change the base?
Conversation
ba2882b
to
7b4b1cd
Compare
Just FTR I have included the same patch in ruby/ruby#8761 where it can be seen that it helps to make the test suite green. However, I have realized that should the ruby/ruby#8761 be accepted, the modified configuration will eventually become default and will be reused for RubyGems test suite resulting in these test cases being always skipped ... |
It seems like the solution to this paradox of permanently skipped tests is to make bundled_gems a built-in feature of rubygems instead of an override. As an aside, the name "bundled gems" seems confusing since it implies connection with bundler, when that is not the intention. "Packaged gems"? 🤷♂️ I'll reply about that to the ruby feature. This name ship sailed a long time ago, but maybe we can avoid naming a directory with this name. Edit: to elaborate on the test situation, I think the test isn't so much testing a default as it is testing how the path is composed from other configuration. If we could standardize how the path is composed no matter what the path was, we could test it just fine with or without a modified path. I think what irks me about the current approach to supplying alternate gem paths is that it's perpetuating and formalizing a monkey patching system. It would be more maintainable to offer an API for this important function with the intention of formalizing all patches into APIs over time. You should be able to push something onto the gem path without overriding anything. |
I agree just partially. The thing is that the Also contrary to "for this important function", I'd say this is quite unimportant function. And the test coverage apparently tests something which might not be used in practice (it certainly is not used on Fedora and the proposed Ruby patch also modifies the functionality in a way that the test is broken). What is important to me is the concept of I am still thinking about proposing something like: $ git diff
diff --git a/lib/rubygems/defaults.rb b/lib/rubygems/defaults.rb
index 1fe6f36f3..ae55a6b7f 100644
--- a/lib/rubygems/defaults.rb
+++ b/lib/rubygems/defaults.rb
@@ -174,9 +174,9 @@ def self.path_separator
def self.default_path
path = []
- path << user_dir if user_home && File.exist?(user_home)
+ path << user_dir if user_home
path << default_dir
- path << vendor_dir if vendor_dir && File.directory?(vendor_dir)
+ path << vendor_dir if vendor_dir
path
end or even $ git diff
diff --git a/lib/rubygems/defaults.rb b/lib/rubygems/defaults.rb
index 1fe6f36f3..5647b13fe 100644
--- a/lib/rubygems/defaults.rb
+++ b/lib/rubygems/defaults.rb
@@ -174,9 +174,9 @@ def self.path_separator
def self.default_path
path = []
- path << user_dir if user_home && File.exist?(user_home)
+ path << user_dir
path << default_dir
- path << vendor_dir if vendor_dir && File.directory?(vendor_dir)
+ path << vendor_dir
path
end
|
Is there a concern of including paths that may not exist? For example, we don't allow gems to be written to world writable directories, for good reason. Do we need to check that none of the paths are world writeable before including them since that could pose a security threat? Beyond that I don't see any reason not to include them. Should rubygems trim off non existent path at runtime and save the result so that each invocation uses only the paths that existed at the time? I otherwise support the idea of adding all dirs by default if it solves the problem and creates no new problems. |
RubyGems provides means to override their defaults, such as `Gem.default_path` method. With such modified configuration, the following errors might be observed: ~~~ 1) Failure: TestGem#test_default_path_vendor_dir [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:612]: <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/gemhome", "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/vendor/gems/3.3.0+0"]> expected but was <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/gemhome", "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/vendor/gems/3.3.0+0", "/usr/local/lib/ruby/bundled_gems/3.3.0+0", "/usr/local/lib/ruby/default_gems/3.3.0+0"]>. make: *** [uncommon.mk:943: yes-test-all] Error 4 2) Failure: TestGem#test_default_path_missing_vendor [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:592]: <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-c41yb7/gemhome"]> expected but was <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-c41yb7/gemhome", "/usr/local/lib/ruby/bundled_gems/3.3.0+0", "/usr/local/lib/ruby/default_gems/3.3.0+0"]>. 3) Failure: TestGem#test_default_path_user_home [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:600]: <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/userhome/.local/share/gem/ruby/3.3.0+0", "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/gemhome"]> expected but was <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/userhome/.local/share/gem/ruby/3.3.0+0", "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/gemhome", "/usr/local/lib/ruby/bundled_gems/3.3.0+0", "/usr/local/lib/ruby/default_gems/3.3.0+0"]>. 4) Failure: TestGem#test_default_path [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:582]: <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-kan9o5/gemhome"]> expected but was <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-kan9o5/gemhome", "/usr/local/lib/ruby/bundled_gems/3.3.0+0", "/usr/local/lib/ruby/default_gems/3.3.0+0"]>. ~~~ Of course the `Gem.default_path` could be stubbed, but testing stub should not be the point. Therefore, just detect if `Gem.default_path` was customized and ignore the test cases.
7b4b1cd
to
4cc0094
Compare
RubyGems provides means to override their defaults, such as
Gem.default_path
method. With such configuration, the following errors might be observed:Of course the
Gem.default_path
could be stubbed, but testing stub should not be the point.Therefore, just detect if
Gem.default_path
was customized and ignore the test cases.This is going to address the last 4 test failures referred in #7119 / ruby/ruby#8761 and I can't see other way around 🫤