-
Notifications
You must be signed in to change notification settings - Fork 161
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
catch if we attempted to send metrics before connfiguring #400
Conversation
add a test that would have caught the view tracker race conditiion
@@ -20,6 +20,7 @@ class ViewTracker | |||
|
|||
def initialize(options = {}) | |||
raise NotImplementedError, "View Tracker requires Rails 4 or greater" unless self.class.supported_version? | |||
raise "Coverband: view tracker initialized before configuration!" if !Coverband.configured? && ENV["COVERBAND_TEST"] == "test" |
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.
Why do we only want to raise this error for ENV["COVERBAND_TEST"] == "test"
?
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.
I was worried about this raising in various scenarios like disabling coverband in RAILS env test or not having a redis in some CI build step, so I wanted to start with just something that let us test and verify the fix... I will keep looking if we can turn it on more extensively
shutdown_server | ||
end | ||
|
||
test "check view tracker" do |
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.
This is great. One thought here and probably not very important. Could use use capybara to hit a remote server here? I've done this before:
Capybara.run_server = false
Capybara.app_host = http://localhost:9999/
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.
interesting, I hadn't thought of that, it would be nice to use capybara visit and asset helpers vs CURL.
I can give it a go
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.
Obviously not so important but i had done it before in a project so figure would mention.
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.
hmm tried several things, basically rack-test driver doesn't support it then hit various issues when trying to use selenium with run_server = false
... seems promising but requires some tweaking
add a test that would have caught the view tracker race condition
How to test fully isolated Coverband Rails Initialization
Follow up on the previous race condition fix.
I found that I couldn't test the Coverband configuration in full isolation with Capybara... I still built off the forked tests which should help keep the test code isolated, but I ended up forking off an entire server and then testing against that server.
Error Output
If I run this code against Coverband before the fix was committed this is how the test fails...