Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Increase test coverage #186

Merged
merged 3 commits into from Feb 27, 2014

Conversation

Projects
None yet
3 participants
Contributor

ArturG commented Feb 27, 2014

More tests! More!

Member

PragTob commented Feb 27, 2014

Hey looks good! You are on a roll man!

For most of the specs I don't get why there has to be a mac user agent background though - I mean for the dl I get it but for the rest... ?

Contributor

ArturG commented Feb 27, 2014

Hi,

in our StaticController we had the following code:

def platform
  if Rails.env.test?
    "mac"
  else
    request.user_agent.match(/Mac|Linux|Windows/).try(:[], 0).try(:downcase)
  end
end

after refactoring we got the following:

def platform
  request.user_agent.downcase
end

So we had to define User-Agent for our test cases, but you are right, the way I solved this problem is not optimal. I'll commit improved version soon.

Contributor

ArturG commented Feb 27, 2014

@PragTob done :)

Coverage Status

Coverage decreased (-14.37%) when pulling 2cebc66 on ArturG:coverage into 211605c on hacketyhack:master.

Member

PragTob commented Feb 27, 2014

Hey there,

thanks! I believe that we do not need that user agent modification for all the specs/features. Although you are right that technically it would be the better refactoring/keeping the old behavior.

I believe it's only needed for checking that the right download is there so we could implement it as a background there so it is only set for that feature scenario and not for all of them :)

If I get to it before you I'll do it (should be ina couple of hours or tomorrow/this weekend) - thanks!

Tobi

Contributor

ArturG commented Feb 27, 2014

You're right and I understand your point. Unfortunately, we cannot ignore this user-agent problem for other scenarios, because then we will get:

Failing Scenarios:
cucumber features/answers.feature:5 # Scenario: Create an answer
cucumber features/answers.feature:13 # Scenario: Edit an answer
cucumber features/blog.feature:17 # Scenario: Post to the blog
cucumber features/moderator.feature:8 # Scenario: Delete a question
cucumber features/moderator.feature:12 # Scenario: Normal users cannot moderate
cucumber features/programs.feature:9 # Scenario: View my programs
cucumber features/questions.feature:5 # Scenario: Create a question
cucumber features/questions.feature:11 # Scenario: Update a question
cucumber features/signup.feature:5 # Scenario: Create an account via the signup form
cucumber features/users.feature:8 # Scenario: View my profile

Because in case, when we do not define user-agent we get the following error (every time when we visit root page):

undefined method `downcase' for nil:NilClass (NoMethodError) 
./app/controllers/static_controller.rb:37:in `platform'
./app/controllers/static_controller.rb:3:in `root'

So there are two ways to solve the problem:

1st, define user-agent for cucumber

features/support/env.rb

Before do
  DatabaseCleaner[:mongo_mapper].clean
  page.driver.header('User-Agent', "Mac")
end

2nd, handle nil user-agent in StaticController

def platform
  request.user_agent.downcase
  request.user_agent.nil? ? nil : request.user_agent.downcase
end

P.S If I'm wrong, please, explain why :)

Coverage Status

Coverage decreased (-14.37%) when pulling 114bc62 on ArturG:coverage into 211605c on hacketyhack:master.

Member

PragTob commented Feb 27, 2014

You are totally right! My bad! Needed that explanation, sorry :)

Just seems odd. I think a better solution would be to adjust the line of code. I think the 2nd solution sounds more robust, we don't want to fail if for some reason the UA is missing :)

Member

PragTob commented Feb 27, 2014

Ah you already did that, awesome I like it!

Btw. great fast feedback loop on this PR :)

PragTob added a commit that referenced this pull request Feb 27, 2014

@PragTob PragTob merged commit b4f74ac into hacketyhack:master Feb 27, 2014

1 check passed

default The Travis CI build passed
Details

@ArturG ArturG deleted the ArturG:coverage branch Feb 27, 2014

Contributor

ArturG commented Feb 27, 2014

Yeah, seems that the time zones difference is not a problem for us :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment