Skip to content
This repository has been archived by the owner on May 3, 2020. It is now read-only.

Rack tests #303

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

Rack tests #303

wants to merge 12 commits into from

Conversation

ccammilleri
Copy link
Member

will build on master and dev branches only.

@BuffaloWill
Copy link
Contributor

@ccammilleri, I am having trouble with the PR. Specifically, when I merge it and run serpico as I normally would (i.e. "ruby serpico.rb") it looks to default to the test cases.

  • Should serpico be initialized differently with test cases in place?

@ccammilleri
Copy link
Member Author

@BuffaloWill good catch! RACK_ENV must be set. I believe i just checked to see if it wasn't set to 'test' but didnt check if it was empty/not set, thus the behavior you're seeing. For tests to run, the webbrick process and the log outputs was causing tests to hang. The environment check was my way to have the tests run against the app without those process interfering.

RACK_ENV=production ruby ./serpico.rb should run the app. If not lmk.

In the past I've solved this by defaulting to a RACK_ENV=production for the app unless overridden via cli. lmk if you'd like that done.

@BuffaloWill
Copy link
Contributor

BuffaloWill commented Aug 4, 2017

I think I figured out the issue. serpico.rb loads all files in the helper directory as part of the initialization in "require ./server.rb":

Dir[File.join(File.dirname(__FILE__), "helpers", "*.rb")].each { |lib| require lib }

The file helpers/test_helper.rb is setting the RACK_ENV to 'test' regardless of the environment setting:
https://github.com/SerpicoProject/Serpico/pull/303/files#diff-ac39cb74eb13a6f4ec9b81b13495b624R1

I tested out adding the following. Worked for both test and prod:

  1. Add the following before the "require ./server.rb":
unless ENV['RACK_ENV']
	ENV['RACK_ENV'] = 'production'
end 
  1. Add to helpers/test.rb:
unless ENV['RACK_ENV']
	ENV['RACK_ENV'] = 'test'
end 

Is that OK as a fix or is there a better approach?

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.

2 participants