-
Notifications
You must be signed in to change notification settings - Fork 96
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
Fix tests failing when config.order set to random #115
Fix tests failing when config.order set to random #115
Conversation
Wasn't being cleaned up / deactivated for some specs, which could leak fakefs activation into other specs depending on order.
Class variable settings were leaking into other specs and wreaking all sorts of havoc.
@@ -12,6 +12,12 @@ | |||
# for some platforms. | |||
@metric_with_graph = MetricFu.configuration.mri? ? :cane : :flay | |||
@metric_without_graph = :hotspots | |||
MetricFu::Configuration.run do |config| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you prefer MetricFu::Configuration.run {} over MetricFu.configuration.configure_metrics {}?
Thanks for picking up this one. |
@robincurry Do you want help with this? |
Possibly. I'll work on it tonight and see if I make any headway. If not Italy need fresh eyes. |
"Italy", "it may", same thing. Thx autocorrect. |
I've turned off autocorrect so my typos are easier to understand :) It does |
Specs now consistently passing for me and on travis. Not sure about commit 3e5881c, but I was consistently seeing undefined method to_json and undefined constant JSON::Parser issues on cruby-2.0-0-p247 on my box, and that cleared it up. |
@@ -35,4 +35,13 @@ def self.tasks_load(tasks_relative_path) | |||
end | |||
|
|||
LOADER.setup | |||
|
|||
def self.reset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a testing concern, only, I'd rather monkey-patch the module in spec/support/metric_monkey.rb or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, went back and forth on that myself.
Awesome! I added some comments, but feel free to merge and address them elsewhere. |
Ok, cool. I'll address these other issues elsewhere as mentioned. Merging. |
Fix tests failing when config.order set to random
Ugh. rbx build failed on merge. Don't know what to make of it. Same seed works fine for me, but I'm running rbx-head - can't figure out how to get the rubinius weekly binaries installed on my mac. |
Fixes #112