Add Support for Rails Engines in Tests

[orionstudt]

Add Support for Rails Engines in Test

See full rundown of issue described here.

This PR will:

1. Add and mount an engine weather to the rails_app example for the purpose of testing
2. Ensure that OAF middleware or the test app wrapper delegates appropriate methods to rails such that rails knows to setup routing helpers for mounted engines

[ahx]

Hi. One problem was the name of the test class (fixed in f7c3548). The other problem was that it always tried to look in the :default API, which is the train-travel-api.

In 4678c8b I told the test to use the registered :attachments API, which solved the issue.

[orionstudt]

Hi @ahx - I'm glad you caught that but this did not fix the issue.

rails test does not test the engines.

You need to run rails test engines/weather to test the engine

[orionstudt]

@ahx once you have run rails test engines/weather you'll see this error:

[17:47:49] josh.studt ~/code/openapi_first/examples/rails_app [joshs/engine-example  *]
$ rails test engines/weather
/Users/josh.studt/code/openapi_first/examples/rails_app/test/test_helper.rb:5:in `block in <main>': undefined method `root' for module Rails (NoMethodError)

  test.register Rails.root.join('../../spec/data/train-travel-api/openapi.yaml')

So mounting an engine has broken the test helper because Rails is now not loaded when your API registration occurs so you can refer to it. Then you should try this arrangement, in order to make sure Rails is loaded:

And you will see this error when you run rails test engines/weather, because Rails is doing something to register a weather_engine method that openapi_first is not doing:

[17:48:08] josh.studt ~/code/openapi_first/examples/rails_app [joshs/engine-example  *]
$ rails test engines/weather
Running 2 tests in a single process (parallelization threshold is 50)
Run options: --seed 21324

# Running:

.E

Error:
Weather::TemperatureControllerTest#test_it_returns_temperature_data_for_given_latitude_and_longitude:
NameError: undefined local variable or method `weather_engine' for an instance of Weather::TemperatureControllerTest

But if you comment out all of the openapi_first setup code:

And then run rails test engines/weather you will see that tests pass:

[17:29:56] josh.studt ~/code/openapi_first/examples/rails_app [joshs/engine-example  *]
$ rails test engines/weather
Running 2 tests in a single process (parallelization threshold is 50)
Run options: --seed 49475

# Running:

..

Finished in 0.069070s, 28.9561 runs/s, 86.8684 assertions/s.
2 runs, 6 assertions, 0 failures, 0 errors, 0 skips

And if you drop a debugger in the the test and run rails test engines/weather you will see that weather_engine is defined like this:

[17:54:40] josh.studt ~/code/openapi_first/examples/rails_app [joshs/engine-example  *]
$ rails test engines/weather
Running 2 tests in a single process (parallelization threshold is 50)
Run options: --seed 42980

# Running:

.[2, 11] in ~/code/openapi_first/examples/rails_app/engines/weather/test/controllers/temperature_controller_test.rb
     2| 
     3| require "test_helper"
     4| 
     5| class Weather::TemperatureControllerTest < ActionDispatch::IntegrationTest
     6|   test "it returns temperature data for given latitude and longitude" do
=>   7|     debugger
     8|     get weather_engine.temperature_path, params: { latitude: "37.7749", longitude: "-122.4194" }
     9| 
    10|     assert_response :success
    11|     json_response = JSON.parse(response.body)
=>#0	block in <class:TemperatureControllerTest> at ~/code/openapi_first/examples/rails_app/engines/weather/test/controllers/temperature_controller_test.rb:7
  #1	block in run (2 levels) at ~/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/minitest-5.25.5/lib/minitest/test.rb:94
  # and 21 frames (use `bt' command for all frames)
(ruby) weather_engine
#<ActionDispatch::Routing::RoutesProxy:0x000000012cab0dd8
 @helpers=#<Module:0x000000012a1ba3c0>,
 @routes=#<ActionDispatch::Routing::RouteSet:0x0000000128c43348>,
 @scope=
  #<#<Class:0x000000012c93a5a8>:0x000000012cd1d350
   @_mock_session=nil,
   @_routes=nil,
   @_weather_engine=#<ActionDispatch::Routing::RoutesProxy:0x000000012cab0dd8 ...>,
   @accept="text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5",
   @app=#<TrainTravel::Application>,
   @controller=nil,
   @host="www.example.com",
   @https=false,
   @named_routes_configured=true,
   @remote_addr="127.0.0.1",
   @request=nil,
   @request_count=0,
   @response=nil,
   @url_options=nil>,
 @script_namer=
  #<Proc:0x000000012af39b68 /Users/josh.studt/.asdf/installs/ruby/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.2.2.1/lib/action_dispatch/routing/mapper.rb:697 (lambda)>>

[ahx]

Thanks for the explaination @orionstudt . I'll have another look.

[orionstudt]

@ahx Alright I found the problem and have a couple options for solutions. I will describe the problem here and then amend this PR as a fix, and start discussions in the places where we can choose which solution is best.

The issue is that the rails implementation of Integration::Runner relies on the routes method being implemented on the app object in test files in order for it to setup routing helpers. You can see that here:

def create_session(app)
    klass = APP_SESSIONS[app] ||= Class.new(Integration::Session) {
      # If the app is a Rails app, make url_helpers available on the session. This
      # makes app.url_for and app.foo_path available in the console.
      if app.respond_to?(:routes) && app.routes.is_a?(ActionDispatch::Routing::RouteSet)
        include app.routes.url_helpers
        include app.routes.mounted_helpers
      end
    }
    klass.new(app)
end

Our wrapper of the app object means that the outer object is an instance of OpenapiFirst::Middlewares::ResponseValidation which does not implement the routes method. Our wrapper has obscured the inner rails app that Integration::Runner looks at to build those helpers. So to fix, we need to expose the inner app's methods on our wrapper object.

[orionstudt]

@ahx This is probably a breaking change. Because of the Rails.root undefined issue the require_relative '../config/environment call now needs to be before any openapi_first test setup.

[orionstudt]

But anybody that mounted an engine at any point would have encountered that bug and needed to change their order.

[orionstudt]

Well, to be clear, nothing will be broken for anyone until they mount an engine. They just will hit the error as soon as they do, and need to change the order here.

[orionstudt]

@ahx This is the potentially more future-proof option - we are forwarding all missing methods to the inner app, in case there is any other missing things that rails relies on that our outer app wrapper is obscuring.

[ahx]

This seems to very rails specific. I am wondering if overwriting "app" is even that useful for rails' integration tests. It just works for basic rack-test tests, but when using rails there seems to be more that is needed to make it work.

I don't think this issue should be solved in the middlewares, but probably inside openapi_first/test. Or maybe in a dedicated openapi_first/rails. What do you think about that?

[orionstudt]

I think that makes sense. We can go with Option #1 for now since it is in openapi_first/test?

[ahx]

Yes. Let's do that.

[orionstudt]

@ahx And this would be a more targeted option, where we delegate only the routes method that we need for this specific case on our wrapper in order to trigger rails to define those helper methods.

[ahx]

I had a quick look at the rails parts, but did not figure out why the integration tests (ActionDispatch::IntegrationTest) need to call app.routes. But that is not important. What I have learned from this is that just overwriting app with a new proc (or plain rack app) is not the way to go here.

Maybe there is some kind of hook? Something like a before, after request, where we can keep app just what it is and focus on the requests/responses in these tests?

I guess this needs a dedicated rails (railtie?) integration, but I am no longer familiar with rails, as we are all in on plain rack and sinatra. So I would need some opinions about this and probably some more help.

[orionstudt]

Rails adds methods to test classes for url_for helpers and mounted rack apps (engines). Since app.routes was not returning a RouteSet due to this lib, it didn't have the information it needs to define those helpers.

I'm open to contributing more as I work on integrating this library with our Rails app. I am leaning toward your library for configurability instead of committee because of recent activity and because it supports things like polymorphic discriminators and the latest version of OpenAPI. But I'd like to get a quick fix in for this (like this Option 1 snippet here) so that I can move forward.

I don't think a rails-specific integration is a bad idea - I think there are other issues here. For instance, now that the errors are fixed try running rails test engines/weather again. You will get a Open API spec failure that says that the weather controller is not returning an object at root, even though it is. I think this is because rails is returning a serialized JSON String instead of a Hash.

[orionstudt]

I did explain why Rails is calling app.routes and link the exact code where it does so here

[ahx]

Thanks for the work on this. The lacking OAS / JSON Schema support of committee was one motivation to start this project. Option 1 sounds like a good start. A PR for that would be appreciated.

[ahx]

Hi. This seems to work now. I have forced-pushed a few things. This looks okay from my point of view. What do you think?

[orionstudt]

Hi. This seems to work now. I have forced-pushed a few things. This looks okay from my point of view. What do you think?

I like the SimpleDelegator. Don't forget to remove the Option 2 methods I added to the middlewares.

I started messing with a custom railtie last night. I'll open a separate PR if I get something that works nicely and is configurable. I'm thinking that if the rails app is configured with the middlewares as an initializer than we probably won't need to use the app wrapper for rails at all, and might want a separate test class for rails.

But I think this is good for now and will unblock me! Any thoughts about the String vs Hash spec error I was hitting?

[ahx]

Any thoughts about the String vs Hash spec error I was hitting?

Yes. I am preparing an issue about right now and would love to get feedback: #418

Tldr; Currently you have to define a response content-type to fix that.

[ahx]

Thanks @orionstudt for the great collaboration. 🚀

[orionstudt]

Thanks @orionstudt for the great collaboration. 🚀

Thank you! I'll report back soon about a railtie.

[orionstudt]

@ahx Hello again!

I wanted to come back and give an update. Underdog is now successfully using openapi_first to validate our Open API spec accuracy during integration testing! This has been very helpful and is going to enable our clients to be more confident using SDKs generated from our Open API documentation.

I did not end up needing to setup a Railtie - we are not actually using the middleware. We decided, for now, to not run this validation at runtime and so did not really have a need for the middleware. We are currently only using it during testing. If we have a need for it I will return to it and give it a look.

I did however have to extend openapi_first with some functionality to get it working for us. We are currently running off of a fork. I'd love if we could amend that and be running from your latest release - so in service of that I have opened 4 PRs that add the functionality that we needed.

I'm optimistic that these features aren't too esoteric and that they generally make sense for your API - and we can of course iterate on them together. I'll link them here:

1. Support Static Path Prefix per Definition #432
2. Add after_response_body_property_validation Hook #433
3. Support running Test Validation after Request Handling #434
4. Support granular Request/Response opt-out of raising errors during testing #435