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 README.md #12

Merged
merged 6 commits into from
Jul 10, 2019
Merged

Update README.md #12

merged 6 commits into from
Jul 10, 2019

Conversation

eliasjpr
Copy link
Contributor

Adds more details about tests

Adds more details about tests
@faustinoaq
Copy link
Contributor

@eliasjpr We should update amber docs as well, see: https://docs.amberframework.org/amber/testing

Copy link
Contributor

@faustinoaq faustinoaq left a comment

Choose a reason for hiding this comment

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

Error: No available formula with the name "chromedriver" 

Looks like there is something wrong with spec dependencies 😅

end
@handler.prepare_pipelines
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question: in reply to an issue here #10 (comment) you suggested a little bit different approach to building controller test class:

class BlogControllerTest < GarnetSpec::Controller::Test
  getter handler : Amber::Pipe::Pipeline
  def initialize
    @handler = Amber::Server.instance.handler
    @handler.prepare_pipelines
  end
end

Isn't it better to use real handler from Amber? I played around with controller specs just yesterday and to me it felt better do to it this way, because for example I changed the pipelane in routes.cr and forgot to change it in tests. Also, if I had multitude of tests it would be painful to have to change every single one of them after changing routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree to an extent but there is a couple of reasons why we decided to do it this way.

  1. To decouple Garnet Spec from amber framework, if we do it the way you suggest above Garnet Spec can only be used with amber
  2. CSRF Handler causes specs to fail and the GarnetSpec::Controller::Test allows you to define the pipeline without certain handlers and the dev has complete control over the pipeline.

I just had the thought of do the following and it would prevent redefining the pipeline all completely but something does not look right

pipeline :web do
    plug Amber::Pipe::PoweredByAmber.new
    # plug Amber::Pipe::ClientIp.new(["X-Forwarded-For"])
    plug Amber::Pipe::Error.new
    plug Amber::Pipe::Logger.new if Amber.env.production?
    plug Amber::Pipe::CSRF.new if Amber.env.production?
    plug Amber::Pipe::Session.new
    plug Amber::Pipe::Flash.new
    plug Amber::Pipe::Reload.new if Amber.env.development?
  end

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the reply. That makes a lot of sense.

Copy link
Member

Choose a reason for hiding this comment

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

An explicit link between garnet and amber doesn't need to be made in order to accomplish this. Garnet is a utility library, Amber should be able to generate specs which utilize it and explicitly link to the production pipeline.

If there is some reason that csrf doesn't work in specs, that should be addressed with the garnet framework, or the way that amber uses garnet. Disabling CSRF is a workaround and is going to cause a bug in some application because the test application is different from the production application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have an idea for this and a PR will follow soon

Copy link
Contributor Author

@eliasjpr eliasjpr Jul 8, 2018

Choose a reason for hiding this comment

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

@robacarp PR #16 does this by only needing to specify the HANDLER=Amber::Server.instance.handler constant in Amber spec_helper.cr which is simpler than the current implementation

module GarnetSpec
  # As you can see there is no need to define a Mock class
  HANDLER = Amber::Server.instance.handler
  # Needed since the server needs to prepare the linked list for each of the pipelines
  HANDLER.prepare_pipelines
end

@faustinoaq faustinoaq dismissed their stale review June 15, 2018 10:00

Travis was fixed

@eliasjpr eliasjpr merged commit a69c8a7 into master Jul 10, 2019
@eliasjpr eliasjpr deleted the ep/update-readme branch July 10, 2019 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants