luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.57k stars 156 forks source link

Update Lucky::BaseHTTPClient to make requests to app directly #1644

Closed matthewmcgarvey closed 2 years ago

matthewmcgarvey commented 2 years ago

Purpose

In Lucky apps, we recommend using Lucky::BaseHTTPClient to make request specs against your running application. It's a simple proxy to an underlying HTTP::Client that allowed using you actions to make requests. In the quest to make our specs as quick as possible, this removes the need to start up the actual server in a different thread for request specs. It takes the middleware of your app can calls them directly.

In a very unofficial test, I saw ~10% speed improvement from the request specs because of this.

This might also aid us in doing something similar for flow specs.

In a normal app, you are generated an ApiClient that looks like this

class ApiClient < Lucky::BaseHTTPClient
  def initialize
    super
    headers("Content-Type": "application/json")
  end

  def self.auth(user : User)
    new.headers("Authorization": UserToken.generate(user))
  end
end

and a separate setup file boots your app in a fiber.

With this change, only one line will be added to your ApiClient and you won't need your app booted anymore

class ApiClient < Lucky::BaseHTTPClient
  app AppServer.new # <--- instructs the client to connect to the app directly

  def initialize
    super
    headers("Content-Type": "application/json")
  end

  def self.auth(user : User)
    new.headers("Authorization": UserToken.generate(user))
  end
end
matthewmcgarvey commented 2 years ago

@jwoertink this has no connection to flow specs. The underlying selenium library makes all requests to update the browser. I'm thinking about adding functionality to flow using something like this, though.

jwoertink commented 2 years ago

Right, but what I mean is if you disabled the running spec server, then Flow specs that need to visit a specific page like localhost:5000/sign_in wouldn't work, right? So for those cases, we couldn't suggest removing this thing https://github.com/luckyframework/lucky_cli/blob/45f35290dae282610ea6aa5012b31861c792a03e/src/web_app_skeleton/spec/setup/start_app_server.cr.ecr#L1-L5

I'm still down for this change. I just want to make sure it's clear to people that there may still be cases where you'll need the running server. When we generate new apps, we may still have to include that bit of code.

matthewmcgarvey commented 2 years ago

O I see what you mean! Yes, you can't remove that until we get flows using an in-memory integration as well.