Also verify if extending would override a class method

Since extending a class means including on the singleton class of the
class, this should now complain this:

``` ruby
module M
  extend Gitlab::Utils::Override

  override :f
  def f
    super.succ
  end
end

class C
  extend M

  def self.f
    0
  end
end
```

It should complain because `C.f` wasn't calling `M#f`.
This should pass verification:

``` ruby
module M
  extend Gitlab::Utils::Override

  override :f
  def f
    super.succ
  end
end

class B
  def self.f
    0
  end
end

class C < B
  extend M
end
```

Because `C.f` would now call `M#f`, and `M#f` does override something.
This commit is contained in:
Lin Jen-Shin 2018-06-04 22:54:50 +08:00
parent 9c29619478
commit f71fc9328c
2 changed files with 123 additions and 67 deletions

View file

@ -87,18 +87,28 @@ module Gitlab
end end
def included(base = nil) def included(base = nil)
return super if base.nil? # Rails concern, ignoring it
super super
queue_verification(base)
end
alias_method :prepended, :included
def extended(mod)
super
queue_verification(mod.singleton_class)
end
def queue_verification(base)
return unless ENV['STATIC_VERIFICATION']
if base.is_a?(Class) # We could check for Class in `override` if base.is_a?(Class) # We could check for Class in `override`
# This could be `nil` if `override` was never called # This could be `nil` if `override` was never called
Override.extensions[self]&.add_class(base) Override.extensions[self]&.add_class(base)
end end
end end
alias_method :prepended, :included
def self.extensions def self.extensions
@extensions ||= {} @extensions ||= {}
end end

View file

@ -1,7 +1,13 @@
require 'spec_helper' require 'fast_spec_helper'
describe Gitlab::Utils::Override do describe Gitlab::Utils::Override do
let(:base) { Struct.new(:good) } let(:base) do
Struct.new(:good) do
def self.good
0
end
end
end
let(:derived) { Class.new(base).tap { |m| m.extend described_class } } let(:derived) { Class.new(base).tap { |m| m.extend described_class } }
let(:extension) { Module.new.tap { |m| m.extend described_class } } let(:extension) { Module.new.tap { |m| m.extend described_class } }
@ -9,6 +15,14 @@ describe Gitlab::Utils::Override do
let(:prepending_class) { base.tap { |m| m.prepend extension } } let(:prepending_class) { base.tap { |m| m.prepend extension } }
let(:including_class) { base.tap { |m| m.include extension } } let(:including_class) { base.tap { |m| m.include extension } }
let(:prepending_class_methods) do
base.tap { |m| m.singleton_class.prepend extension }
end
let(:extending_class_methods) do
base.tap { |m| m.extend extension }
end
let(:klass) { subject } let(:klass) { subject }
def good(mod) def good(mod)
@ -36,7 +50,7 @@ describe Gitlab::Utils::Override do
shared_examples 'checking as intended' do shared_examples 'checking as intended' do
it 'checks ok for overriding method' do it 'checks ok for overriding method' do
good(subject) good(subject)
result = klass.new(0).good result = instance.good
expect(result).to eq(1) expect(result).to eq(1)
described_class.verify! described_class.verify!
@ -45,7 +59,25 @@ describe Gitlab::Utils::Override do
it 'raises NotImplementedError when it is not overriding anything' do it 'raises NotImplementedError when it is not overriding anything' do
expect do expect do
bad(subject) bad(subject)
klass.new(0).bad instance.bad
described_class.verify!
end.to raise_error(NotImplementedError)
end
end
shared_examples 'checking as intended, nothing was overridden' do
it 'raises NotImplementedError because it is not overriding it' do
expect do
good(subject)
instance.good
described_class.verify!
end.to raise_error(NotImplementedError)
end
it 'raises NotImplementedError when it is not overriding anything' do
expect do
bad(subject)
instance.bad
described_class.verify! described_class.verify!
end.to raise_error(NotImplementedError) end.to raise_error(NotImplementedError)
end end
@ -54,7 +86,7 @@ describe Gitlab::Utils::Override do
shared_examples 'nothing happened' do shared_examples 'nothing happened' do
it 'does not complain when it is overriding something' do it 'does not complain when it is overriding something' do
good(subject) good(subject)
result = klass.new(0).good result = instance.good
expect(result).to eq(1) expect(result).to eq(1)
described_class.verify! described_class.verify!
@ -62,7 +94,7 @@ describe Gitlab::Utils::Override do
it 'does not complain when it is not overriding anything' do it 'does not complain when it is not overriding anything' do
bad(subject) bad(subject)
result = klass.new(0).bad result = instance.bad
expect(result).to eq(true) expect(result).to eq(true)
described_class.verify! described_class.verify!
@ -75,83 +107,97 @@ describe Gitlab::Utils::Override do
end end
describe '#override' do describe '#override' do
context 'when STATIC_VERIFICATION is set' do context 'when instance is klass.new(0)' do
before do let(:instance) { klass.new(0) }
stub_env('STATIC_VERIFICATION', 'true')
context 'when STATIC_VERIFICATION is set' do
before do
stub_env('STATIC_VERIFICATION', 'true')
end
context 'when subject is a class' do
subject { derived }
it_behaves_like 'checking as intended'
end
context 'when subject is a module, and class is prepending it' do
subject { extension }
let(:klass) { prepending_class }
it_behaves_like 'checking as intended'
end
context 'when subject is a module, and class is including it' do
subject { extension }
let(:klass) { including_class }
it_behaves_like 'checking as intended, nothing was overridden'
end
end end
context 'when subject is a class' do context 'when STATIC_VERIFICATION is not set' do
subject { derived } before do
stub_env('STATIC_VERIFICATION', nil)
end
it_behaves_like 'checking as intended' context 'when subject is a class' do
end subject { derived }
context 'when subject is a module, and class is prepending it' do it_behaves_like 'nothing happened'
subject { extension } end
let(:klass) { prepending_class }
it_behaves_like 'checking as intended' context 'when subject is a module, and class is prepending it' do
end subject { extension }
let(:klass) { prepending_class }
context 'when subject is a module, and class is including it' do it_behaves_like 'nothing happened'
subject { extension } end
let(:klass) { including_class }
it 'raises NotImplementedError because it is not overriding it' do context 'when subject is a module, and class is including it' do
expect do subject { extension }
let(:klass) { including_class }
it 'does not complain when it is overriding something' do
good(subject) good(subject)
klass.new(0).good result = instance.good
described_class.verify!
end.to raise_error(NotImplementedError)
end
it 'raises NotImplementedError when it is not overriding anything' do expect(result).to eq(0)
expect do described_class.verify!
end
it 'does not complain when it is not overriding anything' do
bad(subject) bad(subject)
klass.new(0).bad result = instance.bad
expect(result).to eq(true)
described_class.verify! described_class.verify!
end.to raise_error(NotImplementedError) end
end end
end end
end end
end
context 'when STATIC_VERIFICATION is not set' do context 'when instance is klass' do
before do let(:instance) { klass }
stub_env('STATIC_VERIFICATION', nil)
end
context 'when subject is a class' do context 'when STATIC_VERIFICATION is set' do
subject { derived } before do
stub_env('STATIC_VERIFICATION', 'true')
end
it_behaves_like 'nothing happened' context 'when subject is a module, and class is prepending it' do
end subject { extension }
let(:klass) { prepending_class_methods }
context 'when subject is a module, and class is prepending it' do it_behaves_like 'checking as intended'
subject { extension } end
let(:klass) { prepending_class }
it_behaves_like 'nothing happened' context 'when subject is a module, and class is extending it' do
end subject { extension }
let(:klass) { extending_class_methods }
context 'when subject is a module, and class is including it' do it_behaves_like 'checking as intended, nothing was overridden'
subject { extension } end
let(:klass) { including_class }
it 'does not complain when it is overriding something' do
good(subject)
result = klass.new(0).good
expect(result).to eq(0)
described_class.verify!
end
it 'does not complain when it is not overriding anything' do
bad(subject)
result = klass.new(0).bad
expect(result).to eq(true)
described_class.verify!
end end
end end
end end