chore(api-server): improve tests (#9325)

This PR improves the tests for the `@redwoodjs/api-server` package. In doing this, I deliberately didn't edit any sources files (save one edit to export an object for testing). My goal was basically to test everything I could. I'm going to be deduplicating code across the framework (a lot of the code in the api-server package was duplicated to avoid breaking anything while iterating on experimental features), and since users can invoke api-server code in a few different ways (`npx @redwoodjs/api-server`, `yarn rw serve`, and importing handlers from `@redwoodjs/api-server`), I don't want to accidentally break anything anywhere. Improving test coverage also revealed some things that need to be fixed. A few high-level notes. Overall, I reduced our reliance on mocking (in many tests, we weren't using fastify at all). Most tests covered `configureFastify` and while a few checked to see that routes were registered correctly, none of them actually started the api-server, or used any of fastify's testing facilities like `inject` to check that the right file was sent back, etc. (Now they do!) Lastly, while most of the tests I added assume fastify, the `dist.test.ts` isn't and is more like a black-box test. Re the "revealed some things that need to be fixed", in no particular order: - `apiUrl` and `apiHost` can be a bit confusing. Even the previous `withApiProxy.test.ts` got them wrong While this test checked if the options were passed correctly, it mixed up the `apiUrl` for the `apiHost`. `apiHost` is the upstream (and should be a URL). https://github.com/redwoodjs/redwood/blob/daaa1998837bdb6eaa42d9160292e781fadb3dc8/packages/api-server/src/__tests__/withApiProxy.test.ts#L51-L54 @Josh-Walker-GM alerted me to this some time ago, but I didn't understand the nuance. Now that I do, these are just poorly named and we should try to fix them. - Everything in web dist is served https://github.com/redwoodjs/redwood/blob/daaa1998837bdb6eaa42d9160292e781fadb3dc8/packages/api-server/src/plugins/withWebServer.ts#L50-L53 We may want to revisit this. I'm sure most of it needs to be served, but it seems like a poor default when it comes to security. Also, it just results in routes being served that don't work. E.g., all prerendered routes are available at their `.html` suffix, but they error out as soon as they're rendered. - The lambda adapter seems a little too simple https://github.com/redwoodjs/redwood/blob/daaa1998837bdb6eaa42d9160292e781fadb3dc8/packages/api-server/src/requestHandlers/awsLambdaFastify.ts#L11-L27 There's a [core fastify plugin for lambdas](https://github.com/fastify/aws-lambda-fastify), and it's much more complicated than our implementation. Should we just use that? What's the difference between our implementation and theirs? - `setLambdaFunctions` should ignore functions that don't export a handler. It probably doesn't do any harm but it registers them as `undefined`: https://github.com/redwoodjs/redwood/blob/daaa1998837bdb6eaa42d9160292e781fadb3dc8/packages/api-server/src/plugins/lambdaLoader.ts#L32-L40 - The lambda adapter supports callback syntax https://github.com/redwoodjs/redwood/blob/daaa1998837bdb6eaa42d9160292e781fadb3dc8/packages/api-server/src/requestHandlers/awsLambdaFastify.ts#L71-L89 I don't think we need to support this anymore. - The CLI handlers don't have help and don't error out on unknown args It's not uncommon to use this CLI standalone so we should polish it. https://github.com/redwoodjs/redwood/blob/daaa1998837bdb6eaa42d9160292e781fadb3dc8/packages/api-server/src/index.ts#L26-L28 ``` $ yarn rw-server --help Options: --help Show help [boolean] --version Show version number [boolean] ``` ``` $ yarn rw-server --foo --bar --baz Starting API and Web Servers... ``` - You can't import `@redwoodjs/api-server` outside a Redwood project This one isn't relevant to users, but it made testing the built package more complicated than it needed to be. The issue is that `getConfig` is called as soon as the file is imported. And if you're not in a redwood project at that point, you're out of luck: https://github.com/redwoodjs/redwood/blob/daaa1998837bdb6eaa42d9160292e781fadb3dc8/packages/api-server/src/cliHandlers.ts#L24 --------- Co-authored-by: Josh GM Walker <56300765+Josh-Walker-GM@users.noreply.github.com>
redwoodjs · Oct 28, 2023 · 8483a57 · 8483a57
1 parent 17b7d5b
commit 8483a57
Show file tree

Hide file tree

Showing 68 changed files with 3,375 additions and 272 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -455,3 +455,44 @@ jobs:
     runs-on: ${{ matrix.os }}
     steps:
       - run: echo "Only doc changes"
+
+  server-tests:
+    needs: check
+
+    name: 📡🔁 server tests / node 18 latest
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: ⬢ Set up Node.js
+        uses: actions/setup-node@v3
+        with:
+          node-version: 18
+
+      - name: 🐈 Set up yarn cache
+        uses: ./.github/actions/set-up-yarn-cache
+
+      - name: 🐈 Yarn install
+        run: yarn install --inline-builds
+        env:
+          GITHUB_TOKEN: ${{ github.token }}
+
+      - name: 🔨 Build
+        run: yarn build
+
+      - name: 🧪 Test
+        run: yarn jest server.test.ts
+        working-directory: ./tasks/server-tests
+        env:
+          REDWOOD_DISABLE_TELEMETRY: 1
+
+  server-tests-docs:
+    needs: only-doc-changes
+    if: needs.only-doc-changes.outputs.only-doc-changes == 'true'
+
+    name: 📡🔁 server tests / node 18 latest
+    runs-on: ubuntu-latest
+
+    steps:
+      - run: echo "Only doc changes"
diff --git a/packages/api-server/.gitignore b/packages/api-server/.gitignore
@@ -0,0 +1,2 @@
+!src/__tests__/fixtures/**/dist
+coverage
diff --git a/packages/api-server/dist.test.ts b/packages/api-server/dist.test.ts
@@ -0,0 +1,105 @@
+import fs from 'fs'
+import path from 'path'
+
+const distPath = path.join(__dirname, 'dist')
+const packageConfig = JSON.parse(fs.readFileSync('./package.json', 'utf-8'))
+
+describe('dist', () => {
+  it("shouldn't have the __tests__ directory", () => {
+    expect(fs.existsSync(path.join(distPath, '__tests__'))).toEqual(false)
+  })
+
+  // The way this package was written, you can't just import it. It expects to be in a Redwood project.
+  it('fails if imported outside a Redwood app', async () => {
+    try {
+      await import(path.join(distPath, 'cliHandlers.js'))
+    } catch (e) {
+      expect(e.message).toMatchInlineSnapshot(
+        `"Could not find a "redwood.toml" file, are you sure you're in a Redwood project?"`
+      )
+    }
+  })
+
+  it('exports CLI options and handlers', async () => {
+    const original_RWJS_CWD = process.env.RWJS_CWD
+
+    process.env.RWJS_CWD = path.join(
+      __dirname,
+      'src/__tests__/fixtures/redwood-app'
+    )
+
+    const mod = await import(
+      path.resolve(distPath, packageConfig.main.replace('dist/', ''))
+    )
+
+    expect(mod).toMatchInlineSnapshot(`
+      {
+        "apiCliOptions": {
+          "apiRootPath": {
+            "alias": [
+              "rootPath",
+              "root-path",
+            ],
+            "coerce": [Function],
+            "default": "/",
+            "desc": "Root path where your api functions are served",
+            "type": "string",
+          },
+          "loadEnvFiles": {
+            "default": false,
+            "description": "Load .env and .env.defaults files",
+            "type": "boolean",
+          },
+          "port": {
+            "alias": "p",
+            "default": 8911,
+            "type": "number",
+          },
+          "socket": {
+            "type": "string",
+          },
+        },
+        "apiServerHandler": [Function],
+        "bothServerHandler": [Function],
+        "commonOptions": {
+          "port": {
+            "alias": "p",
+            "default": 8910,
+            "type": "number",
+          },
+          "socket": {
+            "type": "string",
+          },
+        },
+        "webCliOptions": {
+          "apiHost": {
+            "alias": "api-host",
+            "desc": "Forward requests from the apiUrl, defined in redwood.toml to this host",
+            "type": "string",
+          },
+          "port": {
+            "alias": "p",
+            "default": 8910,
+            "type": "number",
+          },
+          "socket": {
+            "type": "string",
+          },
+        },
+        "webServerHandler": [Function],
+      }
+    `)
+
+    process.env.RWJS_CWD = original_RWJS_CWD
+  })
+
+  it('ships three bins', () => {
+    expect(packageConfig.bin).toMatchInlineSnapshot(`
+      {
+        "rw-api-server-watch": "./dist/watch.js",
+        "rw-log-formatter": "./dist/logFormatter/bin.js",
+        "rw-server": "./dist/index.js",
+      }
+    `)
+  })
+})
diff --git a/packages/api-server/jest.config.js b/packages/api-server/jest.config.js
@@ -0,0 +1,7 @@
+/** @type {import('jest').Config} */
+const config = {
+  testPathIgnorePatterns: ['/node_modules/', '/fixtures/'],
+  coveragePathIgnorePatterns: ['<rootDir>/dist/', '<rootDir>/src/__tests__/'],
+}
+
+module.exports = config
diff --git a/packages/api-server/package.json b/packages/api-server/package.json
@@ -24,7 +24,7 @@
     "build:watch": "nodemon --watch src --ext \"js,jsx,ts,tsx\" --ignore dist --exec \"yarn build && yarn fix:permissions\"",
     "fix:permissions": "chmod +x dist/index.js; chmod +x dist/watch.js",
     "prepublishOnly": "NODE_ENV=production yarn build",
-    "test": "jest src",
+    "test": "jest",
     "test:watch": "yarn test --watch"
   },
   "dependencies": {

diff --git a/packages/api-server/src/__tests__/fastify.test.ts b/packages/api-server/src/__tests__/fastify.test.ts
@@ -0,0 +1,103 @@
+import fastify from 'fastify'
+import { vol } from 'memfs'
+
+import { createFastifyInstance, DEFAULT_OPTIONS } from '../fastify'
+
+// We'll be testing how fastify is instantiated, so we'll mock it here.
+jest.mock('fastify', () => {
+  return jest.fn(() => {
+    return {
+      register: () => {},
+    }
+  })
+})
+
+// Suppress terminal logging.
+console.log = jest.fn()
+
+// Set up RWJS_CWD.
+let original_RWJS_CWD
+const FIXTURE_PATH = '/redwood-app'
+
+beforeAll(() => {
+  original_RWJS_CWD = process.env.RWJS_CWD
+  process.env.RWJS_CWD = FIXTURE_PATH
+})
+
+afterAll(() => {
+  process.env.RWJS_CWD = original_RWJS_CWD
+})
+
+// Mock server.config.js to test instantiating fastify with user config.
+jest.mock('fs', () => require('memfs').fs)
+
+afterEach(() => {
+  vol.reset()
+})
+
+const userConfig = {
+  requestTimeout: 25_000,
+}
+
+jest.mock(
+  '/redwood-app/api/server.config.js',
+  () => {
+    return {
+      config: userConfig,
+    }
+  },
+  {
+    virtual: true,
+  }
+)
+
+jest.mock(
+  '\\redwood-app\\api\\server.config.js',
+  () => {
+    return {
+      config: userConfig,
+    }
+  },
+  {
+    virtual: true,
+  }
+)
+
+describe('createFastifyInstance', () => {
+  it('instantiates a fastify instance with default config', () => {
+    vol.fromNestedJSON(
+      {
+        'redwood.toml': '',
+      },
+      FIXTURE_PATH
+    )
+
+    createFastifyInstance()
+    expect(fastify).toHaveBeenCalledWith(DEFAULT_OPTIONS)
+  })
+
+  it("instantiates a fastify instance with the user's configuration if available", () => {
+    vol.fromNestedJSON(
+      {
+        'redwood.toml': '',
+        api: {
+          'server.config.js': '',
+        },
+      },
+      FIXTURE_PATH
+    )
+
+    createFastifyInstance()
+    expect(fastify).toHaveBeenCalledWith(userConfig)
+  })
+})
+
+test('DEFAULT_OPTIONS configures the log level based on NODE_ENV', () => {
+  expect(DEFAULT_OPTIONS).toMatchInlineSnapshot(`
+    {
+      "logger": {
+        "level": "info",
+      },
+    }
+  `)
+})
diff --git a/packages/api-server/src/__tests__/fixtures/redwood-app-fallback/redwood.toml b/packages/api-server/src/__tests__/fixtures/redwood-app-fallback/redwood.toml
@@ -0,0 +1,21 @@
+# This file contains the configuration settings for your Redwood app.
+# This file is also what makes your Redwood app a Redwood app.
+# If you remove it and try to run `yarn rw dev`, you'll get an error.
+#
+# For the full list of options, see the "App Configuration: redwood.toml" doc:
+# https://redwoodjs.com/docs/app-configuration-redwood-toml
+
+[web]
+  title = "Redwood App"
+  port = 8910
+  apiUrl = "/.redwood/functions" # You can customize graphql and dbauth urls individually too: see https://redwoodjs.com/docs/app-configuration-redwood-toml#api-paths
+  includeEnvironmentVariables = [
+    # Add any ENV vars that should be available to the web side to this array
+    # See https://redwoodjs.com/docs/environment-variables#web
+  ]
+[api]
+  port = 8911
+[browser]
+  open = true
+[notifications]
+  versionUpdates = ["latest"]
diff --git a/packages/api-server/src/__tests__/fixtures/redwood-app-fallback/web/dist/about.html b/packages/api-server/src/__tests__/fixtures/redwood-app-fallback/web/dist/about.html
@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<html lang="en">
+
+<head>
+  <script defer="defer" src="/assets/AboutPage-7ec0f8df.js" type="module"></script>
+  <title data-rh="true">Redwood App | Redwood App</title>
+  <meta charset="UTF-8">
+  <meta name="viewport" content="width=device-width, initial-scale=1.0">
+  <link rel="icon" type="image/png" href="/favicon.png">
+  <script type="module" crossorigin="" src="/assets/index-ff057e8f.js"></script>
+  <link rel="stylesheet" href="/assets/index-613d397d.css">
+  <script> globalThis.__REDWOOD__APOLLO_STATE = {}</script>
+</head>
+
+<body>
+  <!-- Please keep this div empty -->
+  <div id="redwood-app">
+    <header class="relative flex items-center justify-between bg-blue-700 px-8 py-4 text-white">
+      <h1 class="text-3xl font-semibold tracking-tight"><a href="/"
+          class="text-blue-400 transition duration-100 hover:text-blue-100">Redwood Blog</a></h1>
+      <nav>
+        <ul class="relative flex items-center font-light">
+          <li><a href="/about" class="rounded px-4 py-2 transition duration-100 hover:bg-blue-600">About</a></li>
+          <li><a href="/contact" class="rounded px-4 py-2 transition duration-100 hover:bg-blue-600">Contact Us</a></li>
+          <li><a href="/posts" class="rounded px-4 py-2 transition duration-100 hover:bg-blue-600">Admin</a></li>
+          <li><a href="/login" class="rounded px-4 py-2 transition duration-100 hover:bg-blue-600">Log In</a></li>
+        </ul>
+      </nav>
+    </header>
+    <main class="mx-auto mt-3 max-w-4xl rounded-b bg-white p-12 shadow-lg"><!--$-->
+      <p class="font-light">This site was created to demonstrate my mastery of Redwood: Look on my works, ye mighty, and
+        despair!</p>
+      <div id="redwood-announcer"
+        style="position:absolute;top:0;width:1px;height:1px;padding:0;overflow:hidden;clip:rect(0, 0, 0, 0);white-space:nowrap;border:0"
+        role="alert" aria-live="assertive" aria-atomic="true"></div><!--/$-->
+    </main>
+  </div>
+</body>
+
+</html>