Re-enable IDV outage feature spec 🤞

[matthinz]

🛠 Summary of changes

• Re-enable Idv Outage spec disabled in Temporarily skip IdV outage spec #8185, now waiting for pending requests to resolve before tearing down Rails routing table.

Background

A while back, we added a frontend analytics call that fires when the IdV "Welcome" screen is loaded. It turns out these frontend logging API calls can clash with Rails.application.reload_routes!, which is used heavily in the IdV outage feature spec. This clash would result in errors during test runs:

Got 0 failures and 2 other errors:

 1.1) Failure/Error: raise BrowserConsoleLogError.new(javascript_errors) if javascript_errors.present?
      BrowserConsoleLogError:
        Unexpected browser console logging:
        http://127.0.0.1:42645/api/logger - Failed to load resource: the server responded with a status of 500 (Internal Server Error)

...

 1.2) Failure/Error: status, headers, body = @app.call(env)
      ActionController::RoutingError:
        No route matches [POST] "/api/logger"

(See full example.)

Here's the order of events:

1. Outage feature spec runs, and leaves the headless test browser on the Idv "welcome" screen at the end of the test.
2. In an after handler, we call Rails.application.reload_routes! to clean up.
3. While that call is processing, the browser we were using in step 1 (which is still open) makes a POST to /api/logger
4. There is no route, since we haven't finished reloading them yet! The logging request returns a 500 response to the browser
5. The browser logs an error to its console
6. The test ends up failing for two reasons: because the logging request returned a 500 AND there was something unexpected in the browser's console.

To resolve the flakiness, we're adding a line to wait for any pending requests in the test browser to complete before reloading the Rails routes.

[matthinz]

See 58eb833 for how I attempted to reliably simulate the error condition locally.

[matthinz]

If analyticsEndpoint is not set, trackEvent won't make any requests.

[matthinz]

(I'm going to run CI for this branch several times to see if I can get another failure)

[aduth]

Rather than disabling frontend analytics outright, I wonder if there might be some option to either...

1. Wait for network requests to settle in the browser before reloading routes using Capybara built-in helpers
   • e.g. page.server.wait_for_pending_requests prior to Rails.application.reload_routes!
2. Wait for network requests to settle in the browser before reloading routes using some client-side indicator
   • We don't really have anything user-facing to judge unfortunately
   • Could expose some indicator of pending fetch requests either as a DOM global or CSS class, which we monitor in the specs
     • e.g. page.evaluate_async_script('window._pendingTrackedEvents') (not sure I love this 😅 )
3. A bit more extreme, but always worth reevaluating usefulness of our events: Do we still actively monitor the event being tracked, or could it be removed?

[aduth]

I'm also still curious if there's an option which avoids the need to reload the Rails route table, like if what we're defining in config/routes.rb could be handled in a controller instead (common before_action, etc).

[soniaconnolly]

1. Is there a way to make the analytics disabled config only apply in the test environment to avoid affecting prod,
   or
2. Is this something that could happen in prod and we want to protect against it there too

[matthinz]

Wait for network requests to settle in the browser before reloading routes using Capybara built-in helpers
e.g. page.server.wait_for_pending_requests prior to Rails.application.reload_routes!

wait_for_pending_requests is a good suggestion, I will try that. trackEvent may be called after some delay, so I don't think it's guaranteed that the request would be in-flight at the time, but it might be good enough.

I'm also still curious if there's an option which avoids the need to reload the Rails route table, like if what we're defining in config/routes.rb could be handled in a controller instead (common before_action, etc).

Yeah, my main concern with this approach is it means new controllers in the IdV space need to remember to check to see if IdV is disabled before they do anything. We have a lot of controllers, and are adding more and more as we migrate away from the flow state machine. I guess it's not the end of the world, since a lot of these controllers have to remember to do a lot of other checks as well.

[zachmargolis]

LGTM

[aduth]

Nice 👍

[matthinz]

Thanks all, we've had 8 successive green runs of this branch in CI, so I'm going to merge.