Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

Added tests for Reel::Request #227

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

prathmeshranaut
Copy link
Member

@prathmeshranaut prathmeshranaut commented Jun 18, 2016

Added two new tests for Reel::Request class.
@digitalextremist Please check the correct context of the test comment if better position for the tests can be provided.

Code base coverage increased from 91.35% to 91.57% covered.
Coverage for lib/reel/request.rb increased from 97.01% to 100%

client << ExampleRequest.new.to_s
c = connection.detach

expect(c).to be_a Reel::Connection
Copy link
Member Author

Choose a reason for hiding this comment

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

@digitalextremist Do you we can combine line 345 with this line?

Copy link
Member

Choose a reason for hiding this comment

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

Yes @aayushranaut, you can move connection.detach into the expect() call.

Copy link
Member Author

Choose a reason for hiding this comment

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

@digitalextremist Moved connection.detach into expect() call.

@prathmeshranaut
Copy link
Member Author

prathmeshranaut commented Jun 18, 2016

Code base coverage increased from 91.35% to 91.78% covered.
Coverage for lib/reel/connection.rb increased from 94.74% to 96.84%

@prathmeshranaut
Copy link
Member Author

@digitalextremist check for merging
**91.78% to 92.4% overall coverage.
Coverage for lib/reel/connection.rb increased to ••90.91%••

client << ExampleRequest.new.to_s
request = connection.request

expect(request.inspect).to eq '#<Reel::Request GET / HTTP/1.1 @headers={"Host"=>"www.example.com", "Connection"=>"keep-alive", "User-Agent"=>"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_3) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.78 S", "Accept"=>"text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", "Accept-Encoding"=>"gzip,deflate,sdch", "Accept-Language"=>"en-US,en;q=0.8", "Accept-Charset"=>"ISO-8859-1,utf-8;q=0.7,*;q=0.3"}>'
Copy link
Member

Choose a reason for hiding this comment

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

I would pull this user agent especially from a constant @prathmeshranaut. It seems like a good idea to move the ExampleRequest initialization values into a constant, then use the constant in initialize there -- but then also use the constant here in the test. This is a lot of hard-coded ultra-specific information :)

@digitalextremist
Copy link
Member

Once you make the change I mentioned, this is good to pull. Great work @prathmeshranaut

@prathmeshranaut
Copy link
Member Author

@digitalextremist Please take a look.

@@ -31,4 +31,8 @@ def to_s
@headers.map { |k, v| "#{k}: #{v}" }.join("\r\n") << "\r\n\r\n" <<
(@body ? @body : '')
end

def inspect_method
Copy link
Member Author

Choose a reason for hiding this comment

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

I think I could do better with naming this stub. Do you have any suggestions?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants