Merge pull request #642 from guarzo/fix/test-failures-and-ci

fix(ci): make the test job fail when tests fail

.github/workflows/test.yml

@@ -104,8 +104,12 @@ jobs:
       - name: Run tests with coverage
         id: tests
         run: |
-          # Run tests with coverage
-          output=$(mix test --cover 2>&1 || true)
+          # Run tests with coverage. The exit code is captured rather than
+          # discarded: this step must fail the job when the suite is red.
+          set +e
+          output=$(mix test --cover 2>&1)
+          test_exit_code=$?
+          set -e
           echo "$output" > test_output.txt
 
           # Parse test results
@@ -134,11 +138,12 @@ jobs:
           fi
           echo "success_rate=$success_rate" >> $GITHUB_OUTPUT
 
-          exit_code=$?
-          echo "exit_code=$exit_code" >> $GITHUB_OUTPUT
-        continue-on-error: true
+          # Fail the job on a red suite. Reporting steps below still run
+          # because they depend on this step's outputs via always().
+          exit $test_exit_code
 
       - name: Generate coverage report
+        if: always()
         id: coverage
         run: |
           # Generate coverage report with GitHub format
@@ -164,6 +169,7 @@ jobs:
         continue-on-error: true
 
       - name: Run Credo analysis
+        if: always()
         id: credo
         run: |
           # Run Credo and capture output
@@ -201,6 +207,7 @@ jobs:
         continue-on-error: true
 
       - name: Run Dialyzer analysis
+        if: always()
         id: dialyzer
         run: |
           # Ensure PLT is built
@@ -228,6 +235,7 @@ jobs:
         continue-on-error: true
 
       - name: Create test results summary
+        if: always()
         id: summary
         run: |
           # Calculate overall score
@@ -368,3 +376,64 @@ jobs:
           path: pr-comment/
           retention-days: 1
         continue-on-error: true
+
+  # Integration tests are excluded from the default `mix test` run by
+  # test/test_helper.exs, so until now they had never executed in CI at all.
+  # They run here as a separate job rather than by changing the default
+  # exclusion, so local `mix test` stays fast.
+  #
+  # This is a hard gate: a red integration run blocks the merge. Note that 30 of
+  # the tests `--only integration` selects are `@tag :skip`, so the gate covers
+  # the 35 that actually execute.
+  integration-test:
+    name: Integration Tests
+    runs-on: ubuntu-latest
+
+    services:
+      postgres:
+        image: postgres:15
+        env:
+          POSTGRES_PASSWORD: postgres
+          POSTGRES_DB: wanderer_test
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+        ports:
+          - 5432:5432
+
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Setup Elixir/OTP
+        uses: erlef/setup-beam@v1
+        with:
+          elixir-version: ${{ env.ELIXIR_VERSION }}
+          otp-version: ${{ env.OTP_VERSION }}
+
+      - name: Cache Elixir dependencies
+        uses: actions/cache@v3
+        with:
+          path: |
+            deps
+            _build
+          key: ${{ runner.os }}-mix-${{ hashFiles('**/mix.lock') }}
+          restore-keys: ${{ runner.os }}-mix-
+
+      - name: Install Elixir dependencies
+        run: |
+          mix deps.get
+          mix deps.compile
+
+      - name: Setup database
+        run: |
+          mix ecto.create
+          mix ecto.migrate
+
+      - name: Run integration tests
+        # --only, not --include: --include would run the whole suite on top of
+        # the tagged tests, duplicating the main test job's ~10 minutes and
+        # re-reporting failures it already covers.
+        run: mix test --only integration

lib/wanderer_app/api/map.ex

@@ -34,7 +34,10 @@ defmodule WandererApp.Api.Map do
     repo(WandererApp.Repo)
     table("maps_v1")
 
-    migration_defaults scopes: "'{wormholes}'"
+    # This value is injected verbatim into generated migrations. It must be
+    # Elixir source for a list of strings: `'{wormholes}'` is a charlist, which
+    # generated a default of the character codes of the literal "{wormholes}".
+    migration_defaults scopes: ~s(["wormholes"])
   end
 
   json_api do

priv/repo/migrations/20260801200000_fix_maps_scopes_default.exs

@@ -0,0 +1,46 @@
+defmodule WandererApp.Repo.Migrations.FixMapsScopesDefault do
+  @moduledoc """
+  Repairs the `maps_v1.scopes` column default.
+
+  Migrations 20260331192521 and 20260406213852 declared the default as
+  `default: '{wormholes}'`. In Elixir a single-quoted literal is a charlist,
+  i.e. `[123, 119, 111, ...]`, so Ecto emitted an eleven-element text array of
+  the character codes of the literal string `{wormholes}` instead of the
+  one-element array `{wormholes}`.
+
+  Any row inserted without an explicit `scopes` value therefore received
+  garbage that cannot be cast back to `{:array, :atom}`, and every subsequent
+  read of that row failed with `Ash.Error.Unknown` ("cannot load ... as type").
+
+  This migration sets the correct default and repairs rows already written with
+  the bad value.
+  """
+
+  use Ecto.Migration
+
+  # Derived from the offending literal rather than transcribed by hand: a typo
+  # in a hardcoded list would make the WHERE clause match nothing and silently
+  # repair no rows. Both 20260331192521 and 20260406213852 used exactly this
+  # literal, so this single value catches rows written by either.
+  @bad_default Enum.map(~c"{wormholes}", &Integer.to_string/1)
+
+  def up do
+    alter table(:maps_v1) do
+      modify :scopes, {:array, :text}, default: ["wormholes"]
+    end
+
+    execute(
+      "UPDATE maps_v1 SET scopes = ARRAY['wormholes']::text[] WHERE scopes = ARRAY[#{Enum.map_join(@bad_default, ",", &"'#{&1}'")}]::text[]"
+    )
+  end
+
+  # Deliberately not a mirror image of up/0. Restoring the original charlist
+  # default would reintroduce the bug, so this drops the default instead, and
+  # rows already repaired stay repaired. Rolling back therefore leaves the
+  # schema in a different -- but correct -- state rather than the prior one.
+  def down do
+    alter table(:maps_v1) do
+      modify :scopes, {:array, :text}, default: nil
+    end
+  end
+end

priv/resource_snapshots/repo/maps_v1/20260801200000.json

@@ -0,0 +1,277 @@
+{
+  "attributes": [
+    {
+      "allow_nil?": false,
+      "default": "fragment(\"gen_random_uuid()\")",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": true,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "id",
+      "type": "uuid"
+    },
+    {
+      "allow_nil?": false,
+      "default": "nil",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "name",
+      "type": "text"
+    },
+    {
+      "allow_nil?": false,
+      "default": "nil",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "slug",
+      "type": "text"
+    },
+    {
+      "allow_nil?": true,
+      "default": "nil",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "description",
+      "type": "text"
+    },
+    {
+      "allow_nil?": true,
+      "default": "nil",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "personal_note",
+      "type": "text"
+    },
+    {
+      "allow_nil?": true,
+      "default": "nil",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "public_api_key",
+      "type": "text"
+    },
+    {
+      "allow_nil?": true,
+      "default": "[]",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "hubs",
+      "type": [
+        "array",
+        "text"
+      ]
+    },
+    {
+      "allow_nil?": false,
+      "default": "\"wormholes\"",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "scope",
+      "type": "text"
+    },
+    {
+      "allow_nil?": true,
+      "default": "false",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "deleted",
+      "type": "boolean"
+    },
+    {
+      "allow_nil?": true,
+      "default": "false",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "only_tracked_characters",
+      "type": "boolean"
+    },
+    {
+      "allow_nil?": true,
+      "default": "nil",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "options",
+      "type": "text"
+    },
+    {
+      "allow_nil?": false,
+      "default": "false",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "webhooks_enabled",
+      "type": "boolean"
+    },
+    {
+      "allow_nil?": false,
+      "default": "false",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "sse_enabled",
+      "type": "boolean"
+    },
+    {
+      "allow_nil?": true,
+      "default": "[\"wormholes\"]",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "scopes",
+      "type": [
+        "array",
+        "text"
+      ]
+    },
+    {
+      "allow_nil?": false,
+      "default": "fragment(\"(now() AT TIME ZONE 'utc')\")",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "inserted_at",
+      "type": "utc_datetime_usec"
+    },
+    {
+      "allow_nil?": false,
+      "default": "fragment(\"(now() AT TIME ZONE 'utc')\")",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": null,
+      "scale": null,
+      "size": null,
+      "source": "updated_at",
+      "type": "utc_datetime_usec"
+    },
+    {
+      "allow_nil?": true,
+      "default": "nil",
+      "generated?": false,
+      "precision": null,
+      "primary_key?": false,
+      "references": {
+        "deferrable": false,
+        "destination_attribute": "id",
+        "destination_attribute_default": null,
+        "destination_attribute_generated": null,
+        "index?": false,
+        "match_type": null,
+        "match_with": null,
+        "multitenancy": {
+          "attribute": null,
+          "global": null,
+          "strategy": null
+        },
+        "name": "maps_v1_owner_id_fkey",
+        "on_delete": null,
+        "on_update": null,
+        "primary_key?": true,
+        "schema": null,
+        "table": "character_v1"
+      },
+      "scale": null,
+      "size": null,
+      "source": "owner_id",
+      "type": "uuid"
+    }
+  ],
+  "base_filter": null,
+  "check_constraints": [],
+  "custom_indexes": [],
+  "custom_statements": [],
+  "has_create_action": true,
+  "hash": "F00091B5D3467DFD9D0B5DD8E5A5590E581D1F4D51F0D6CD07413358DC1AF2B7",
+  "identities": [
+    {
+      "all_tenants?": false,
+      "base_filter": null,
+      "index_name": "maps_v1_unique_public_api_key_index",
+      "keys": [
+        {
+          "type": "atom",
+          "value": "public_api_key"
+        }
+      ],
+      "name": "unique_public_api_key",
+      "nils_distinct?": true,
+      "where": null
+    },
+    {
+      "all_tenants?": false,
+      "base_filter": null,
+      "index_name": "maps_v1_unique_slug_index",
+      "keys": [
+        {
+          "type": "atom",
+          "value": "slug"
+        }
+      ],
+      "name": "unique_slug",
+      "nils_distinct?": true,
+      "where": null
+    }
+  ],
+  "multitenancy": {
+    "attribute": null,
+    "global": null,
+    "strategy": null
+  },
+  "repo": "Elixir.WandererApp.Repo",
+  "schema": null,
+  "table": "maps_v1"
+}

test/integration/map/map_scope_filtering_test.exs

@@ -213,6 +213,17 @@ defmodule WandererApp.Map.MapScopeFilteringTest do
       to_solar_system_id: @ls_system_halmah
     })
 
+    # These "jump_*" keys live in the global cache and are never invalidated
+    # once written -- CachedInfo.get_solar_system_jump/2 only rebuilds the index
+    # when a key is missing. MapScopesTest asserts on the same system IDs
+    # (30_000_001 / 30_000_002 / 30_000_100) expecting NO stargate, so leaving
+    # these behind made its "valid when no stargate exists" cases fail depending
+    # on whether this suite happened to run first.
+    on_exit(fn ->
+      WandererApp.Cache.delete(halenan_mili_key)
+      WandererApp.Cache.delete(halenan_halmah_key)
+    end)
+
     :ok
   end
 

test/unit/controllers/auth_controller_test.exs

@@ -5,7 +5,7 @@ defmodule WandererAppWeb.AuthControllerTest do
 
   describe "parameter validation and error handling" do
     test "callback/2 validates missing assigns" do
-      conn = build_conn()
+      conn = browser_conn()
       params = %{}
 
       # Should handle gracefully when required assigns are missing
@@ -34,7 +34,7 @@ defmodule WandererAppWeb.AuthControllerTest do
     test "callback/2 handles malformed auth data gracefully" do
       # Test with minimal conn structure to exercise error paths
       # The callback/2 function will match the fallback clause and redirect
-      conn = build_conn()
+      conn = browser_conn()
 
       result = AuthController.callback(conn, %{})
 
@@ -46,7 +46,7 @@ defmodule WandererAppWeb.AuthControllerTest do
     test "callback/2 processes auth structure with missing fields" do
       # Test the fallback clause since auth structure is incomplete
       # Missing CharacterOwnerHash will cause pattern match failure
-      conn = build_conn()
+      conn = browser_conn()
 
       result = AuthController.callback(conn, %{})
 
@@ -58,7 +58,7 @@ defmodule WandererAppWeb.AuthControllerTest do
     test "callback/2 exercises character creation path" do
       # Test the fallback clause for now since character creation involves complex validation
       # The actual implementation requires valid EVE character data which is complex to mock
-      conn = build_conn()
+      conn = browser_conn()
 
       result = AuthController.callback(conn, %{})
 
@@ -69,7 +69,7 @@ defmodule WandererAppWeb.AuthControllerTest do
 
     test "callback/2 handles existing user assignment" do
       # Test the fallback clause for consistent behavior
-      conn = build_conn()
+      conn = browser_conn()
 
       result = AuthController.callback(conn, %{})
 
@@ -81,8 +81,8 @@ defmodule WandererAppWeb.AuthControllerTest do
     test "callback/2 validates various auth credential formats" do
       # Test fallback clause behavior for various cases
       test_cases = [
-        build_conn(),
-        build_conn() |> assign(:some_other_assign, "value")
+        browser_conn(),
+        browser_conn() |> assign(:some_other_assign, "value")
       ]
 
       Enum.each(test_cases, fn conn ->
@@ -192,4 +192,15 @@ defmodule WandererAppWeb.AuthControllerTest do
       end)
     end
   end
+
+  # The SSO callback failure path calls put_flash/3, which requires the session
+  # and flash plugs that the real :browser pipeline installs. A bare
+  # build_conn/0 has neither, so these tests died with "flash not fetched"
+  # before ever reaching the redirect they assert on. This mirrors the browser
+  # pipeline rather than relaxing the controller.
+  defp browser_conn do
+    build_conn()
+    |> Plug.Test.init_test_session(%{})
+    |> Phoenix.ConnTest.fetch_flash()
+  end
 end

test/unit/map/map_scopes_test.exs

@@ -70,6 +70,19 @@ defmodule WandererApp.Map.Server.MapScopesTest do
       Cachex.put(:system_static_info_cache, solar_system_id, system_info)
     end)
 
+    # :system_static_info_cache is global and shared with every other suite in
+    # the run. These stub entries carry only :solar_system_id and :system_class,
+    # so leaving them behind silently replaces the full records other suites
+    # expect -- 30_000_142 (Jita) in particular is also used by
+    # CommonAPIControllerTest and OpenAPIValidationTest, which then read a
+    # record missing solar_system_name and friends. Whether that surfaced
+    # depended on suite order, which is why these failures moved with --seed.
+    on_exit(fn ->
+      Enum.each(Map.keys(test_systems), fn solar_system_id ->
+        Cachex.del(:system_static_info_cache, solar_system_id)
+      end)
+    end)
+
     :ok
   end