Auto test improvements, simplification, cleanup

[p-mcgowan]

The idea with the auto tests was to make writing tests easier, but the end result was a bit too complicated - some people didn't know which test method to use or why more than 1 existed.

Would like to simplify this in a second pass - the idea is to trim out the stuff that was not being used, and cut down on the complexity.

currently, this is an example test output:

test helper suite - nodegene/tests/domain.ts:

import {
  baseUrl,
  request,
  TestParams,
  TestRequest,
} from '@/http/nodegen/tests';
import * as supertest from 'supertest';
import { pathKid, responseValidator } from './PublicKeysDomain.data';

export const publicKeysGet: TestRequest = {
  specName: 'can GET /public-keys',
  testKey: 'publicKeysGet',
  getPath: (testParams?: TestParams, root = baseUrl): string =>
    `${root}/public-keys`,
  getRequest: (testParams?: TestParams, root = baseUrl): supertest.Test =>
    request.get(`${root}/public-keys`),
  request: (testParams?: TestParams, root = baseUrl): supertest.Test =>
    request.get(`${root}/public-keys`).expect(({ status, body }) => {
      if (status !== (testParams?.statusCode ?? 200)) {
        console.error(body);
      }
      expect(status).toBe(testParams?.statusCode ?? 200);
      expect(body).toBeDefined();
      const validated = responseValidator(
        `publicKeysGet${testParams?.statusCode ?? 200}`,
        body
      );
      if (validated.error) {
        console.error(validated.error);
      }
      expect(!!validated.error).toBe(false);
    }),
};

export const publicKeysKidGet: TestRequest = {
  specName: 'can GET /public-keys/{kid}',
  testKey: 'publicKeysKidGet',
  getPath: (testParams?: TestParams, root = baseUrl): string =>
    `${root}/public-keys/${testParams?.path?.kid ?? pathKid}`,
  getRequest: (testParams?: TestParams, root = baseUrl): supertest.Test =>
    request.get(`${root}/public-keys/${testParams?.path?.kid ?? pathKid}`),
  request: (testParams?: TestParams, root = baseUrl): supertest.Test =>
    request
      .get(`${root}/public-keys/${testParams?.path?.kid ?? pathKid}`)
      .expect(({ status, body }) => {
        if (status !== (testParams?.statusCode ?? 200)) {
          console.error(body);
        }
        expect(status).toBe(testParams?.statusCode ?? 200);
        expect(body).toBeDefined();
        const validated = responseValidator(
          `publicKeysKidGet${testParams?.statusCode ?? 200}`,
          body
        );
        if (validated.error) {
          console.error(validated.error);
        }
        expect(!!validated.error).toBe(false);
      }),
};

and the output tests - domains/tests/domain.api.spec.ts:

import {
  defaultSetupTeardown,
  TestParams,
  TestPublicKeysDomain,
} from '@/http/nodegen/tests';

defaultSetupTeardown();

describe('PublicKeysDomain', () => {
  // setup - run before suite (one time)
  beforeAll(async () => {});

  // setup - run before every test
  beforeEach(async () => {});

  // teardown - run after every tests
  afterEach(async () => {});

  // teardown - run once after suite succeeds
  afterAll(async () => {});

  it('can GET /public-keys', async () => {
    const testParams: TestParams = {};
    await TestPublicKeysDomain.tests.publicKeysGet.request(testParams);
  });

  it('can GET /public-keys/{kid}', async () => {
    const testParams: TestParams = {};
    await TestPublicKeysDomain.tests.publicKeysKidGet.request(testParams);
  });
});

having the default happy path tester and the getRequest method ended up being more complicated than it needed to be, so I'm proposing something simpler:

nodegen/tests/domain:

import { baseUrl, request, TestParams, TestRequest } from '@/http/nodegen/tests';
import * as supertest from 'supertest';
import { pathKid, responseValidator } from './PublicKeysDomain.data';

export class TestPublicKeysDomain {
  // publicKeysGet
  //
  public static publicKeysGetPath(testParams?: TestParams): string {
    return `/public-keys`;
  }

  public static publicKeysGet(testParams?: TestParams, root = baseUrl): supertest.Test {
    return request.get(`${root}/public-keys`);
  }

  // publicKeysKidGet
  //
  public static publicKeysKidGetPath(testParams?: TestParams): string {
    return `/public-keys/${testParams?.path?.kid ?? pathKid}`;
  }

  public static publicKeysKidGet(testParams?: TestParams, root = baseUrl): supertest.Test {
    return request.get(`${root}/public-keys/${testParams?.path?.kid ?? pathKid}`);
  }
}

and the auto-gen test suite

domains/tests/domain.api.spec:

import {
  defaultSetupTeardown,
  TestParams,
  TestPublicKeysDomain,
} from '@/http/nodegen/tests';
import { responseValidator } from '@/http/nodegen/tests/PublicKeysDomain.data';

defaultSetupTeardown();

describe('PublicKeysDomain', () => {
  it('can GET /public-keys', async () => {
    const testParams: TestParams = {};
    await TestPublicKeysDomain.publicKeysGet(testParams).expect(
      ({ status, body }) => {
        expect(status).toBe(200);
        expect(body).toBeDefined();
        const validated = responseValidator(
          `publicKeysGet${testParams?.statusCode ?? 200}`,
          body
        );
        if (validated.error) {
          console.error(validated.error);
        }
        expect(!!validated.error).toBe(false);
      }
    );
  });

  it('can GET /public-keys/{kid}', async () => {
    const testParams: TestParams = {};
    await TestPublicKeysDomain.publicKeysKidGet(testParams).expect(
      ({ status, body }) => {
        expect(status).toBe(200);
        expect(body).toBeDefined();
        const validated = responseValidator(
          `publicKeysKidGet${testParams?.statusCode ?? 200}`,
          body
        );
        if (validated.error) {
          console.error(validated.error);
        }
        expect(!!validated.error).toBe(false);
      }
    );
  });
});

The main difference being the default happy path test is moved from the helper suite into the spec (so that the user can just delete it when they start writing more useful tests), and the test suite contains just a few helpers in a less-nested format.

[p-mcgowan]

Also note - 100% not backwards compatible. Since the names and exports have changed, and the test suites are stubbed (not overwritten) this means anyone who has been using the old pattern of test imports will have built errors.

Without building some complicated upgrade path rewriter, not much we can do about it. As far as I know, very few users currently use the auto tests, but it means everyone that does will start seeing errors

[j-d-carmichael]

From my side backward compat will be easier to fix - don't know about acr side though. From mem i think @SergiAcrontum was using this on his project.

[p-mcgowan]

Yeah I was also considering leaving the old stuff in and adding a deprecation warning on import just in case - then maybe we can tag an issue with deprecated and remove it in another version.

Was also toying with the idea of removing the whole test wrapper and just dumping test boilerplate into the spec files just using the request directly - The idea with the wrapper is that the routes can change and it won't break the tests, but if it's back to the old stub-vs-rebuild issue then I think there will be issues either way.

I can push up a POC for playing with and we can see how it fits into the various code-bases, but I think the main benefit for these auto tests is a few mock helpers and test files and a guide for testing strategies, but for existing projects with DB wired up and all that you end up rewriting the specs anyway.

[j-d-carmichael]

I mean maybe you could add a flag in the RC file of this tpl repo - the flag would instruct the use of the current version and lack of flag would just use the newer code eg:

OLD: src/http/nodegen/tests/___eval.ts (only outputs when rc flag is set & dumps a deprecation warning to the console)
NEW: src/http/nodegen/tests/api/___eval.ts (only outputs when rc flag is not set)

[p-mcgowan]

sounds like it might get complicated. Technically, the user can prevent regeneration of tests by removing the test section of the RC as-is, which isn't ideal but at least it's an option.

I think realistically if we are not going to build a migration tool into the CLI for the time being, and the impact of the change is fairly small (probably only a handful of people using the auto tests), we could safely just pull the trigger and get people to either make the changes in the codebase, remove the test section in the rc, or just hold back or lock the template upgrade.

However, without getting too deep into it the auto tests were a first pass anyway, and I assume anyone who is writing api tests will have quickly outgrown the stubs already, so the overall impact would be pretty low to begin with.

Ill slap something together and play around with it to see if there is an easy upgrade path. It should be mostly just a find and replace kinda deal, so I suspect most people wouldn't be too bothered. Could almost bake it in but wouldn't want to start building the migration thing just yet.

[j-d-carmichael]

Coolios