Shared tests for potential violations (stubbing non-existent/public methods, etc.) #460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
force-pushed
the
stubbing-check-shared-tests
branch
from
3ce847c
to
8f6a9b4
Compare
nitishr
changed the title
nitishr
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Also, the two PRs together get rid of all the rubocop Lint/DuplicateMethod disables.