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

Reduce matcher interface #468

Merged
merged 1 commit into from Nov 7, 2015
Merged

Reduce matcher interface #468

merged 1 commit into from Nov 7, 2015

Conversation

mbj
Copy link
Owner

@mbj mbj commented Oct 30, 2015

  • New primary interface #call makes specs and implementations much
    easier
  • We had #each mostly for historical reasons that are not relevant
    anymore
  • Kills mutations in Mutant::Matcher* namespace that had been present because it was written before --zombie was functional.

Extraction from #446

* New primary interface #call makes specs and implementations much
  easier
* We had #each mostly for historical reasons that are not relevant
  anymore
* Mutation covers the Mutant::Matcher namespace
@mbj mbj force-pushed the fix/reduce-matcher-interface branch 9 times, most recently from 9d5f379 to bfbc3bc Compare October 31, 2015 00:51
@mbj
Copy link
Owner Author

mbj commented Oct 31, 2015

@dkubb Assuming all green this is ready for review.

Sorry this change is so big. I initially thought about constraining it into the Matcher namespace and the call side in Env::Bootstrap but I found a blatand LSP violation that created a much bigger backpressure to also touch Env::Bootstrap.

Lets say it this way: The size of this change is paying legacy debt.

@mbj
Copy link
Owner Author

mbj commented Oct 31, 2015

This can be validated for:

  • Mutation coverage of the change
  • Not breaking mutant

Via:

bundle exec mutant --zombie -I lib -r mutant --use rspec \ 
  'Mutant::Env::Bootstrap*' \
  'Mutant::Expression*' \
  'Mutant::Matcher*' 

This is more coarse grained than the check on CI that does via --since a more fine grained validation.

@mbj mbj assigned dkubb Nov 1, 2015
@@ -95,7 +95,7 @@ def scope_name(scope)
# @api private
def infect
config.includes.each(&$LOAD_PATH.method(:<<))
config.requires.each(&method(:require))
config.requires.each(&Kernel.method(:require))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious. Why did you add Kernel?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short: To be able to spec the global side effect this code should trigger.

Long:

When specifying requires, its IMO okay to rely on the fact Kernel#require will do the right thing when called. So its IMO fine to place message expectations to "specify" certain requires happen.

I could actually have setup a real file require it and observe a constant was added, but that would defeat the idea of a unit spec. A unit spec under execution should not mutate global state. (OT: Thats a nice way to try defining unit spec vs integration spec!)

Now when setting up message expectations (that are temporal monkey patches effectively) we need an object to target this monkey patches on.

2 thoughts arise from that:

  • The primary API of this class is Bootstrap.call that returns Env. No instance of Bootstrap is visible to the API client. The whole intention is to cause side effects and produce a "stable" environment object. Tests should only use the API of the class and do not "fuck around" with their implementation details. As I do not have access on an instance of Bootstrap via the API I do not have an object to set the message expectation up against.
  • If I'd even have an API where I could reach instances of Bootstrap setting up a message expectation on the object under test would be a very bad idea. Here I actually have to blame ruby because very important APIs with global side effects such as Kernel#require are available as private methods everywhere. I'd prefer it if require 'foo' would only work toplevel, or at class level, but not at instance level per default. This would reduce lots of confusion.

In summary: By changing the code to another construct, causing the same side effect. Its now more testable, as I can setup the message expectation on Kernel that is globally available.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another maybe more useful instance of the same pattern is available here: https://github.com/mbj/mutant/blob/master/bin/mutant#L13-L29

This is the call side for mutants self-zombification. It explicitly passes kernel into the zombifier API, for testing the zombifier I could implement a "fake Kernel" (that also does some message expectations, but not via rspec) to specify the behavior of Zombification.

Here is the test support class: https://github.com/mbj/mutant/blob/master/spec/support/ruby_vm.rb

This example is an instance of the same idea: When multiple APIs that produce the same effect exist (#require vs Kernel.require) use the one that is easier to unit test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think next refactorings will pass in Kernel, $stderr, $stdout and other global friends explicitly into the mutant namespace. I had great success with that on the Mutant::Zombifier sub-project. So it'll help everywhere.

Each time I have to place a message expectation via rspec on a global constant something in my hart dies.

@mbj mbj force-pushed the fix/reduce-matcher-interface branch 8 times, most recently from 6b17438 to 2fbc362 Compare November 7, 2015 13:17
@mbj mbj force-pushed the fix/reduce-matcher-interface branch 2 times, most recently from de31179 to 8233c58 Compare November 7, 2015 22:57
mbj added a commit that referenced this pull request Nov 7, 2015
@mbj mbj merged commit 2b9554b into master Nov 7, 2015
@mbj mbj deleted the fix/reduce-matcher-interface branch November 7, 2015 23:05
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.

None yet

4 participants