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

Update RSpec timing adaptor to be more resilient #64

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

aterris
Copy link
Contributor

@aterris aterris commented Aug 24, 2017

Overview

In a project where we are using Knapsack, we noticed that some tests were incorrectly reporting a much shorter time than they were actually taking.

After some investigation, I noticed that it was because we had Knapsack::Adapters::RSpecAdapter.bind placed at the bottom of our spec_helper.rb

Above this, we had anRSpec.configure block that contained a slow after(:each) block that was adding about 1 second per test - doing DB work.

Because before aliases append_before and after aliases prepend_after, if the bind happens after the config of these blocks, the after block that contains the end of the timing calculation is executed before the other config block.

http://www.rubydoc.info/github/rspec/rspec-core/RSpec%2FCore%2FConfiguration:after

Example Repo

I have pushed a minimal app that shows the issue here
https://github.com/aterris/knapsack-issue

Safer Implementation

While we did not match the prescribed documentation (by putting bind at the bottom instead of top of spec_helper), it feels like this is an easy mistake for other users to make and is also something we can help protect against.

RSpec provides prepend_before and append_after for extension type work that would fix this in most cases (including the sample app above). It is still possible to hit this if the user is using those methods directly, but this change would make that much less likely since users are not often using them. These methods were added in 2.10 as seen in the changelog

I have updated the rspec_adaptor_spec.rb but have not added a test specifically around this case. I can look into that but hadn't found what felt like a good way to test this yet.

Final Note

I actually noticed this bug originally with Knapsack Pro but ended up just making this PR here for now. If you are happy with this change, I can make at PR on pro as well, or you can do it yourself, whichever you prefer.

@ArturT
Copy link
Member

ArturT commented Aug 24, 2017

@aterris Good catch! Thanks for PR. I will look into this and I will do changes in knapsack_pro.

@ArturT
Copy link
Member

ArturT commented Aug 25, 2017

Here is fix in knapsack_pro KnapsackPro/knapsack_pro-ruby#52

@ArturT
Copy link
Member

ArturT commented Aug 25, 2017

@aterris I've released a new version of knapsack and knapsack_pro gem. Thanks for help!

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

2 participants