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

Shared tests for potential violations (stubbing non-existent/public methods, etc.) #460

Open
wants to merge 63 commits into
base: main
Choose a base branch
from

Conversation

nitishr
Copy link
Contributor

@nitishr nitishr commented Jan 15, 2020

Very similar to #458. In fact, there's overlap between the two that can be cleaned up once/if we merge both of them.

The idea behind both of these PRs is to reveal

  • the underlying themes and similarities/variations on a number of axes - method owner (class, superclass, module, etc.), stubbee/stub_owner (class, instance, any_instance) and visibility (public, protected, private), and configuration/treatment (allow, warn, prevent) of various stubbing actions (non-existent method, non-public method, method on nil, etc.)
  • that the purpose of the whole bunch of tests if to really check a small number of things for a larger number of scenarios combining elements of the axes mentioned above.

Also, the two PRs together get rid of all the rubocop Lint/DuplicateMethod disables.

This allows us to call stub_method_unnecessarily in the default case without a
treatment
if RUBY_VERSION < '2.2.0', there's no point defining a test class that
does just setup and teardown. Instead, let's not define the test class
at all.
... following the same structure and logic as in the case of
stubbing_method_unnecessarily
...to reduce diff between StubbingMethod{Unnecessarily,OnNil}Test to
prep to extract common module
...to furter reduce diff betwee StubbingMethod{Unnecessarily,OnNil}Test
to prep to extract common module
...to furter reduce diff betwee StubbingMethod{Unnecessarily,OnNil}Test
to prep to extract common module
This seems to have been an artifact of copy-paste
... pulling up all the identical methods between
Stubbing{MethodUnnecessarily,Nil}Test
We can now start using the 'framework' we've extracted in the form of a
shared test module customizable via configure_violation,
potenial_violation and message_on_violation.
StubbingNonExistentAnyInstanceMethodTest varies only in those 3 things.
It checks the same things as in the shared module (viz. allow, warn,
prevent and default).
... just as in the case of StubbingNonExistentAnyInstanceMethodTest
... just as in case of StubbingNonExistent{AnyInstance,Class}MethodTest
... just as in all the StubbingNonExistent*MethodTest
so we can include the module rather than a test calling the same
assertion in multiple test classes
... to reduce diff between and prep to DRY up
test_should_allow_stubbing_existing_*_any_instance_method
... this will allow us to extract a parameterizable method to DRY up
... to prep for extracting both into a single method
... since those tests don't use the same fixture and 'framework' as the
tests for checking stubbing non-existent methods, and the class and
superclass tests follow the same structure, so separating them
highlights the similarity and affords an opportunity for abstraction.
... between
test_should_allow_stubbing_existing_*_any_instance_{,super}class_method
... by specifying custom method_owner and stub_owner
from AllowStubbingNonExistent{AnyInstance,Class,Instance}{,Superclass}MethodTest
after a rename to make it more generic. This also adds the respond_to? check to
superclass tests. It wasn't clear why we should check for existing methods on a
class and its superclass, but skip the superclass check for a method to which we
the object responds
Just as in the case of stubbing non-existent method tests, we break the
single StubbingNonPublicAnyInstanceMethodTest into:

- StubbingNonPublicAnyInstanceMethodTest - with just
test_should_allow_stubbing_public_any_instance_method
- StubbingPrivateAnyInstanceMethodTest - checks that stubbing private
methods is checked (allow/warn/prevent)
- StubbingProtectedAnyInstanceMethodTest - checks that stubbing private
methods is checked (allow/warn/prevent)
by using the same method names, and setting visibility programmatically
rather than declaratively
... in StubbingNonPublic{Class,Instance}MethodTest
for use in StubbingNonPublic{Class,Instance}MethodTest with overridable
method_owner and stubbed_owner later
following the same structure as StubbingNonPublicAnyInstanceMethodTest
- StubbingNonPublicClassMethodTest
- StubbingPrivateClassMethodTest
- StubbingProtectedClassMethodTest
following the same structure as
StubbingNonPublic{AnyInstance,Class}MethodTest
- StubbingNonPublicInstanceMethodTest
- StubbingPrivateInstanceMethodTest
- StubbingProtectedInstanceMethodTest
and use it in StubbingNonPublic{AnyInstance,Class,Instance}MethodTest.
This also adds the check for stubbing method that's responded to for
AnyInstance since it's more consistent with the other two and it wasn't
clear why the check didn't exist for AnyInstance while it did for Class
and Instance methods
... and to make duplication more obvious
@nitishr nitishr changed the title Stubbing check shared tests Shared tests for checking potential violations (stubbing non-existent/public methods, etc.) Jan 16, 2020
@nitishr nitishr changed the title Shared tests for checking potential violations (stubbing non-existent/public methods, etc.) Shared tests for potential violations (stubbing non-existent/public methods, etc.) Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant