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

fix(tests): Prevent router_test.exs from running async #2288

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

joshlarson
Copy link
Contributor

Most tests rely on the HOST environment variable to be set to nil, so that it defaults to localhost. If HOST is set to www.mbta.com, that causes a redirect. router_test.exs has a test that validates that redirect, by setting HOST to www.mbta.com, validating the redirect, and then setting it back to nil.

This is what that normally looks like in context:

sequenceDiagram
  RouterTest->>Env: set_env("HOST", "www.mbta.com")
  RouterTest->>Env: get_env("HOST")
  Env-->>RouterTest: "www.mbta.com"
  RouterTest->>Env: set_env("HOST", nil)
  OtherTest->>Env: get_env("HOST")
  Env-->>OtherTest: nil
Loading

(Note the existence of OtherTest, which also retrieves HOST, and relies on HOST being nil.)

BUT, when router_test.exs's execution is interleaved with other tests, then sometimes another test will try to retrieve HOST (well, another test will invoke a conn, which then invokes HOST through the CanonicalHostname plug) before router_test.exs has set HOST back to nil.

sequenceDiagram
  RouterTest->>Env: set_env("HOST", "www.mbta.com")
  RouterTest->>Env: get_env("HOST")
  Env-->>RouterTest: "www.mbta.com"
  OtherTest->>Env: get_env("HOST")
  Env-->>OtherTest: "www.mbta.com"
  RouterTest->>Env: set_env("HOST", nil)
Loading

The conn invoked by OtherTest gets the www.mbta.com HOST and does a redirect, which the other test wasn't expecting, and fails.

This creates a very challenging debugging environment, because, in this context, OtherTest could refer to literally any other test that makes an HTTP request or does anything with a conn. This bug was causing very rare non-deterministic failures in every other HTTP-related test.

The solution here is to remove the async: true flag from router_test.exs, preventing it from interleaving itself with other tests.

Hat-tip to @thecristen for helping me figure out that this CanonicalHostname plug was the culprit.

@joshlarson joshlarson requested a review from a team as a code owner December 21, 2024 00:10
@joshlarson joshlarson enabled auto-merge (squash) December 21, 2024 00:10
@joshlarson joshlarson merged commit ebed273 into main Dec 23, 2024
20 checks passed
@joshlarson joshlarson deleted the jdl/fix-evil-router-test branch December 23, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants