Skip to content
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

Doesn't highlight error for missing constant when const_missing is redefined for class #34

Open
luke-gru opened this issue Mar 2, 2023 · 7 comments

Comments

@luke-gru
Copy link
Contributor

luke-gru commented Mar 2, 2023

I'm working on new feature for ruby's test/unit tool and I define Test::Unit::TestCase.const_missing. It does some stuff then calls old const_missing like the following (pseudo code):

orig = self.method(:const_missing)
def const_missing(name)
  # do some stuff 
   orig.call(name)
end

This broke error_highlight tests for missing constant, it wasn't highlighting anymore. I'm not sure if this is fixable or not.

@mame
Copy link
Member

mame commented Mar 2, 2023

Can you please write down the executable code and exactly what you expect?

class Foo
  $orig = self.method(:const_missing)
  def self.const_missing(name)
    $orig.call(name)
  end
end

Foo::Bar

Current:

$ ruby test.rb
test.rb:5:in `call': uninitialized constant Foo::Bar (NameError)

    $orig.call(name)
         ^^^^^
        from test.rb:5:in `const_missing'
        from test.rb:9:in `<main>'

Maybe what you expect:

$ ruby test.rb
test.rb:5:in `call': uninitialized constant Foo::Bar (NameError)

Foo::Bar
   ^^^^^
        from test.rb:5:in `const_missing'
        from test.rb:9:in `<main>'

or

$ ruby test.rb
test.rb:9:in `<main>': uninitialized constant Foo::Bar (NameError)

Foo::Bar
   ^^^^^

The former can be achieved with error_highlight. But Foo::Bar is not in test.rb:5, so it may be confusing. The latter is quite difficult to achieve with error_highlight.

(I was aware of a similar problem, but had not written about it before, so I have created #35.)

@luke-gru
Copy link
Contributor Author

luke-gru commented Mar 2, 2023

If you go in Ruby repository and at the top of the test_error_highlight.rb you put:

klass = Test::Unit::TestCase
def klass.const_missing(name)
  super
end

you will see failing tests.

@mame
Copy link
Member

mame commented Mar 2, 2023

Sorry, I don't understand the context at all. Why do you need to run the test of error_highlight along with your hack for test-unit gem? Are you proposing to introduce your hack into the test-unit gem?

@luke-gru
Copy link
Contributor Author

luke-gru commented Mar 3, 2023

Yeah it's a bit confusing, I'll explain. The changes I'm making are not to the test-unit gem, but to ruby's internal testing framework that is used when running ruby's test-suite (make test-all). You can find the file that I'm changing in the ruby repository at tool/lib/test/unit/testcase.rb. When I was changing this file to include my hack, all the tests in ruby's test suite were still working except for error_highlight tests. I was thinking that maybe this is a bug in error_highlight itself, maybe it does not work properly when the class (or the parent class) has defined const_missing. That is why I reported the issue. To reproduce the issue, you would have to go to the ruby repository and run make test-all TESTOPTS="test/error_highlight/test_error_highlight.rb" with the changes in my 2nd comment in this thread.

@mame
Copy link
Member

mame commented Mar 3, 2023

I don't know why you try to change tool/lib/test/unit/testcase.rb, but anyway.

In fact, error_highlight does not work well when const_missing is defined. This is a variant problem of #35. I think this is a limitation around exceptions in Ruby itself rather than an issue with error_highlight.
If tool/lib/test/unit/testcase.rb defines Test::Unit::TestCase.const_missing, I will remove the method temporarily during error_highlight testing.

On another note, I don't think it's a good idea to define Test::Unit::TestCase.const_missing in tool/lib/test/unit/testcase.rb. make test-all should run the test suite in a bare Ruby environment as much as possible. This is one of the reasons why Ruby test suite has not migrated to rspec.

@luke-gru
Copy link
Contributor Author

luke-gru commented Mar 3, 2023

I was just messing around with this, and got it working.

in def self.spot in base.rb:

        return nil unless loc
        if NameError === obj
          while locs.any? && /in `const_missing'/ =~ loc.to_s
            locs.shift
            loc = locs.first
          end
        end

Kind of hacky, but gets the job done 😄

As for not changing test/unit/testcase.rb, I understand your concerns.

@mame
Copy link
Member

mame commented Mar 3, 2023

Your patch is what I said as 2-1 in #35. Its problem is that the line number shown by the backtrace and the snippet shown by error_highlight are discrepant.

class Foo
  def self.const_missing(_)
    super
  end

  Bar
end
$ ruby -I lib/ test.rb
test.rb:3:in `const_missing': uninitialized constant Foo::Bar (NameError)

  Bar
  ^^^
        from test.rb:6:in `<class:Foo>'
        from test.rb:1:in `<main>'

Note that there is no Bar in test.rb:3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants