Project

General

Profile

Misc #16235

ENV.assoc spec test does not test invalid name

Added by burdettelamar@yahoo.com (Burdette Lamar) about 2 months ago. Updated about 2 months ago.

Status:
Open
Priority:
Normal
Assignee:
-
[ruby-core:95205]

Description

The most important thing here is an added test for an invalid name argument to ENV.assoc, which should raise TypeError.

I've also added to spec_helper.rb:

  • Methods :reserve_names and :release_names, to reserve and release names used in the test.
  • Method :mock_to_str, to return a mock object that responds to :to_str.

The updated assoc_spec.rb uses all of these, as will many other spec tests for ENV, as I get to them.

It's known that some of the ENV spec tests do not test a method's return value, and that some do not test error conditions (as above), so more to come. All in good time.


Files

t.diff (2.24 KB) t.diff burdettelamar@yahoo.com (Burdette Lamar), 10/03/2019 09:04 PM

History

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

It appears if reserve_names is called with a name that is already an environment variable, release_names will remove that name from the environment, which seems like a bug. Now, that issue exists in the current code (environment variable foo is always modified and removed. But if we are going to handle this, we should probably not set @reserved until after checking the names, and only remove the @reserved entries if the instance variable is defined.

Note that since these changes are only to the specs, you may want to consider submitting them as pull requests to ruby/spec: https://github.com/ruby/spec/pulls

Updated by burdettelamar@yahoo.com (Burdette Lamar) about 2 months ago

Method :reserve_names fails if any name is already in use:

def reserve_names(*names)
  @reserved = names
  @reserved.each  do |name|
    fail "Name #{name} is already in use" if ENV.include?(name)
  end
end

So, not a problem?

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

burdettelamar@yahoo.com (Burdette Lamar) wrote:

Method :reserve_names fails if any name is already in use:

def reserve_names(*names)
  @reserved = names
  @reserved.each  do |name|
    fail "Name #{name} is already in use" if ENV.include?(name)
  end
end

So, not a problem?

reserve_names is called in a before :each block. When it fails, the after :each block is called. That block calls release_names which will delete the entry from the environment. In my opinion, if you are checking whether the environment variable is already set, and it is already set, and the spec fails because of this, you probably should not remove the entry from the environment. Otherwise you are making the spec fail for no reason.

Updated by burdettelamar@yahoo.com (Burdette Lamar) about 2 months ago

Got it, thanks. Will update the diff.

Updated by burdettelamar@yahoo.com (Burdette Lamar) about 2 months ago

Have put this up as a PR, so we're done here.

Also available in: Atom PDF