Make the test environment match production and let before_all work in specs (#814)

Author: sdogruyol

CHANGELOG.md

@@ -17,6 +17,13 @@ error 405 do |env|
 end
 ```
 
+- Make the `test` environment behave like every other one, and let `before_all`/`after_all` work in specs [#788](https://github.com/kemalcr/kemal/issues/788). Three things differed between a spec run and production:
+
+  - An unmatched route answered an empty `200` under `KEMAL_ENV=test`. `Kemal.run` registered the default 404 page only outside that environment, and without a registered `error 404` a `RouteNotFound` rendered nothing. The page is now registered in every environment, and a `RouteNotFound` with no handler renders a plain `404 Not Found` like the other built-in errors do.
+  - `before_all` ran for an unmatched path — a 404, a 405, or a WebSocket path reached without an upgrade — only when a custom `error 404`/`405` handler was registered, which `Kemal.run` did in production and nothing did under spec. An authentication guard in `before_all` therefore held in production and not in the tests that were supposed to prove it. It runs for every unmatched path now, in every environment.
+  - Kemal's top-level `before_all` and `after_all` shadow the `describe`-level hooks of the `spec` library, so a spec calling `before_all { seed }` inside a `describe` registered a request filter and never ran the block, with no error. Called inside a `describe` they now register the spec hook; called anywhere else — an application file, a route, an example — they register Kemal's filter as before.
+
+  Anything asserting an empty `200` for an unknown path in the test environment, or relying on `before_all` not running for unmatched paths under spec, has to be updated.
 - Build the `Content-Disposition` of `send_file` per [RFC 6266](https://www.rfc-editor.org/rfc/rfc6266#section-4.3) and [RFC 8187](https://www.rfc-editor.org/rfc/rfc8187). The filename was dropped into the quoted-string as it was: a `"` in it ended the parameter early, a non-ASCII name went out raw where the parameter is defined as ASCII, and a control character made the standard library reject the header with a `500`. `"` and `\\` are now escaped, other characters outside printable ASCII become `_` in `filename`, and when that loses anything the original name follows as `filename*=UTF-8''…`, which user agents prefer. A plain ASCII name produces the same header as before. `Kemal::Utils.content_disposition` is the builder.
 - `params.raw_body` returns the body of any request, not only a form or JSON one. It came back empty for `text/plain`, XML, or a request with no `Content-Type` at all, so such a body looked absent rather than unread; it is now read and cached the same way, under `max_request_body_size`. A `multipart/form-data` body is the one exception and still returns `""`, since `parse_files` streams it part by part. JSON detection now goes by media type instead of a string prefix: `application/vnd.api+json` and other `+json` types ([RFC 6839](https://www.rfc-editor.org/rfc/rfc6839)) parse into `params.json`, `Application/JSON` matches, and `application/jsonp` no longer does.
 - Skip the WebSocket upgrade when a `before` filter has already answered. A `halt` in a `before_all` — an authentication check answering `401` — closed the response, and `Kemal::WebSocketHandler` went on to attempt the upgrade regardless; the standard library handler raised `IO::Error: Closed stream` on the closed response. The client had its `401`, but every rejected handshake was logged as a server error. The handler now returns once it finds the response closed.

README.md

@@ -210,6 +210,22 @@ Kemal.config.show_exceptions = false
 
 Register `error 500` to render your own page instead; the setting only decides between Kemal's two built-in ones.
 
+**Can I use `before_all` in my specs?**
+
+Yes. Kemal's `before_all` and `after_all` share their names with the `describe`-level hooks of Crystal's `spec` library, and as top-level definitions they take precedence. Called inside a `describe` block they act as the spec hooks — the block runs once around the group's examples — so a spec file reads the same with or without Kemal loaded. Called anywhere else, including inside an example, they register Kemal's filter.
+
+```crystal
+describe "Users" do
+  before_all { seed_users }   # the spec hook: once, before the examples
+
+  it "lists them" do
+    before_all { |env| env.set "user", "bob" }   # Kemal's filter, for this example's requests
+    get "/users" { |env| env.get("user").to_s }
+    call_request_on_app(HTTP::Request.new("GET", "/users")).body.should eq("bob")
+  end
+end
+```
+
 **Does Kemal work with any ORM?**
 
 Yes. You can use any Crystal ORM or database library. No forced dependencies.

spec/exception_handler_spec.cr

@@ -18,6 +18,20 @@ describe "Kemal::ExceptionHandler" do
     response.status_code.should eq 404
   end
 
+  it "answers an unmatched route with 404 through the full chain without a custom handler" do
+    # Without a registered `error 404` this used to fall through as an empty
+    # `200` - which is what every request in the `test` environment got, since
+    # only `Kemal.run` outside it registered the page.
+    get "/" do
+      "Hello"
+    end
+
+    response = call_request_on_app(HTTP::Request.new("GET", "/nope"))
+    response.status_code.should eq 404
+    response.headers["Content-Type"].should eq "text/plain"
+    response.body.should eq "Not Found"
+  end
+
   it "does not reflect the request in the message a 404 handler receives" do
     # Echoing `ex.message` is the obvious thing to write in an `error 404`
     # handler, and the response is `text/html`, so the message must not carry

spec/filters_spec.cr

@@ -251,4 +251,33 @@ describe "Kemal::FilterHandler" do
       client_response.body.should eq(body)
     end
   end
+
+  it "runs before_all for an unmatched path whether or not an error handler is registered" do
+    # `Kemal.run` registers an `error 404` outside the `test` environment and
+    # nothing does inside it; `before_all` used to run only when one existed, so
+    # an authentication guard ran in production and not under spec.
+    Kemal.config.error_handlers.has_key?(404).should be_false
+
+    filter_handler = Kemal::FilterHandler.new
+    filter_handler._add_route_filter("ALL", "*", :before) do |env|
+      env.response.headers["X-Seen"] = "1"
+    end
+    Kemal.config.add_filter_handler(filter_handler)
+
+    response = call_request_on_app(HTTP::Request.new("GET", "/nope"))
+    response.status_code.should eq(404)
+    response.headers["X-Seen"].should eq("1")
+  end
+
+  it "lets before_all answer an unmatched path itself" do
+    filter_handler = Kemal::FilterHandler.new
+    filter_handler._add_route_filter("ALL", "*", :before) do |env|
+      halt env, status_code: 401, response: "auth required"
+    end
+    Kemal.config.add_filter_handler(filter_handler)
+
+    response = call_request_on_app(HTTP::Request.new("GET", "/nope"))
+    response.status_code.should eq(401)
+    response.body.should eq("auth required")
+  end
 end

spec/run_spec.cr

@@ -143,6 +143,13 @@ describe "Run" do
       CR
   end
 
+  it "registers the default 404 page in the test environment too" do
+    run(<<-'CR').should contain("404 handler: true")
+      Kemal.run { }
+      puts "404 handler: #{Kemal.config.error_handlers.has_key?(404)}"
+      CR
+  end
+
   it "runs without a block being specified" do
     run(<<-CR).should contain "[test] Kemal is running in test mode."
       Kemal.run

spec/spec_hooks_spec.cr

@@ -0,0 +1,52 @@
+require "./spec_helper"
+
+# Kemal's top-level `before_all`/`after_all` shadow the `describe` hooks of the
+# spec library. Inside a `describe` they must behave as the spec hooks: run once
+# around the group's examples, never register a request filter.
+AFTER_ALL_RAN = [] of Symbol
+
+describe "before_all inside a describe" do
+  runs = 0
+
+  before_all do
+    runs += 1
+  end
+
+  after_all do
+    AFTER_ALL_RAN << :ran
+  end
+
+  it "has run once before the first example" do
+    runs.should eq(1)
+  end
+
+  it "does not run again for the next example" do
+    runs.should eq(1)
+  end
+
+  it "registered no request filter" do
+    # A filter would have run for this request and bumped the counter.
+    get("/") { "ok" }
+    call_request_on_app(HTTP::Request.new("GET", "/")).body.should eq("ok")
+    runs.should eq(1)
+  end
+end
+
+describe "after_all inside a describe" do
+  it "has run once the previous group finished" do
+    AFTER_ALL_RAN.should eq([:ran])
+  end
+end
+
+describe "before_all outside a describe body" do
+  it "registers a request filter when called from an example" do
+    # Examples run after every `describe` block has been evaluated, so this is
+    # not a `describe` body; it is Kemal's filter, as in an application file.
+    before_all do |env|
+      env.response.headers["X-Filtered"] = "1"
+    end
+    get("/") { "ok" }
+
+    call_request_on_app(HTTP::Request.new("GET", "/")).headers["X-Filtered"].should eq("1")
+  end
+end

src/kemal.cr

@@ -42,11 +42,9 @@ module Kemal
     config.setup
     config.port = port if port
 
-    # Test environment doesn't need to have signal trap and logging.
-    if config.env != "test"
-      setup_404
-      setup_trap_signal if trap_signal
-    end
+    setup_404
+    # A test environment does not listen, so it has no signal to trap.
+    setup_trap_signal if trap_signal && config.env != "test"
 
     server = config.server ||= HTTP::Server.new(config.handlers)
 

src/kemal/dsl.cr

@@ -147,10 +147,28 @@ end
 #
 # NOTE: Response headers must be set before the response body is written,
 # so use `before_*` filters for header changes.
+#
+# `before_all` and `after_all` share their names with the `describe`-level hooks
+# of Crystal's `spec` library, and as top-level definitions they win. Called
+# directly inside a `describe` block they therefore register the spec hook, as
+# a spec author expects (see `Kemal::SpecHooks`); called anywhere else - the top
+# level of a file, a route, an example - they register Kemal's filter.
 {% for type in ["before", "after"] %}
   {% for method in FILTER_METHODS %}
     def {{ type.id }}_{{ method.id }}(path : String = "*", &block : HTTP::Server::Context -> _)
-     Kemal::FilterHandler::INSTANCE.{{ type.id }}({{ method }}.upcase, path, &block)
+      {% if method == "all" %}
+        filter = block
+        {% if @top_level.has_constant?("Spec") %}
+          unless Spec.cli.current_context.is_a?(Spec::RootContext)
+            # A spec hook takes no argument; the filter block was typed to take a
+            # context, so it gets a blank one it will never look at.
+            return Spec::KemalHooks.{{ type.id }}_all do
+              filter.call(HTTP::Server::Context.new(HTTP::Request.new("GET", "/"), HTTP::Server::Response.new(IO::Memory.new)))
+            end
+          end
+        {% end %}
+      {% end %}
+      Kemal::FilterHandler::INSTANCE.{{ type.id }}({{ method }}.upcase, path, &block)
     end
 
     def {{ type.id }}_{{ method.id }}(paths : Enumerable(String), &block : HTTP::Server::Context -> _)

src/kemal/exception_handler.cr

@@ -7,7 +7,7 @@ module Kemal
     def call(context : HTTP::Server::Context)
       call_next(context)
     rescue ex : Kemal::Exceptions::RouteNotFound
-      call_exception_with_status_code(context, ex, 404)
+      call_fixed_status(context, ex, 404)
     rescue ex : Kemal::Exceptions::MethodNotAllowed
       call_method_not_allowed(context, ex)
     rescue ex : Kemal::Exceptions::CustomException

src/kemal/filter_handler.cr

@@ -40,12 +40,14 @@ module Kemal
     # The call order of the filters is `before_all -> before_x -> X -> after_x -> after_all`.
     def call(context : HTTP::Server::Context)
       if !context.route_found?
-        # A request that matched no route can still reach a custom `error`
-        # handler - 404, or 405 when the path is routed for another method - and
-        # that handler expects the same `before_all` setup a route gets.
-        if Kemal.config.error_handlers.has_key?(404) || Kemal.config.error_handlers.has_key?(405)
-          call_block_for_path_type("ALL", context.request.path, :before, context)
-        end
+        # A request that matched no route still gets the `before_all` filters: it
+        # ends in a 404 or 405 page, or in a WebSocket upgrade, and the setup those
+        # expect - an authentication check above all - is the same a route gets.
+        # This used to depend on whether a custom `error 404`/`405` handler was
+        # registered, which `Kemal.run` does outside the `test` environment and
+        # nothing does inside it, so a `before_all` guard ran in production and
+        # not under spec.
+        call_block_for_path_type("ALL", context.request.path, :before, context)
         return call_next(context)
       end