Tests do not enforce the Pipeline interface contract; invalid keys are serialised into YAML

[neilherbertuk]

While working on my PR, I noticed an issue that significantly reduces the effectiveness of the current test suite, particularly given that this is a Typescript package.

At a high level: the tests validate string output, but they do not validate that the input object actually conforms to the intended Pipeline interface shape.

Example

Using the existing minimal pipeline test as a base, lets add a keyword that is not part of the interface. I've added one to globalKeywords :

test('minimal pipeline', () => {
  const minimalPipeline: Pipeline = {
    globalKeywords: {
      workflow: {
        name: 'Minimal Pipeline',
      },
      totallyInvalidKey: {
        anotherInvalidKey: 'Invalid Key Value'
      },
    },
    jobs: {},
  };

  equal(toYAML(minimalPipeline), readFileSync('./test/minimalPipeline.yaml', 'utf8'));
});

My IDE correctly identifies this as invalid;

When this test runs, I would expect the input to cause a compile time / type error as it does not conform to the Pipeline interface. In reality, the test fails but not because of this, it is because the totallyInvalidKey has been silently added to the output, which the snapshot it is being compared to does not include.

Updating the yaml snapshot results in the test passing:

workflow:
  name: Minimal Pipeline
totallyInvalidKey:
  anotherInvalidKey: Invalid Key Value

My understanding is that the built-in node test runner (node:test) has no knowledge of Typescript so the interfaces and types are being ignored.

I would like to suggest that the package switches to using jest with ts-jest.

Migrating the above test to jest:

import { readFileSync } from 'node:fs';
import { Pipeline, toYAML } from '../src/index.ts';

describe('minimal pipeline', () => {
  it('should output valid yaml', () => {
    const minimalPipeline: Pipeline = {
      globalKeywords: {
        workflow: {
          name: 'Minimal Pipeline',
        },
        totallyInvalidKey: {
          anotherInvalidKey: 'Invalid Key Value'
        },
      },
      jobs: {},
    };

    expect(toYAML(minimalPipeline)).toEqual(readFileSync('./test/minimalPipeline.yaml', 'utf8'));
  });
});

Running jest against this results in:

Removing the invalid key and reverting the snapshot allows the test to pass:

[onlywei]

The unit test may not catch this, but I expect the build script does since it runs tsc. The build script is run in ci.yml as well, did it not catch this for you?

[neilherbertuk]

The current tests aren't really testing anything meaningful, they only check that toYAML can serialise an object. Invalid or unexpected keys can pass through completely unnoticed, creating a "garbage in, garbage out" scenario. While downstream projects would fail to compile because TypeScript enforces the interface, this isn't very helpful for developers making changes to the package itself, since the test environment doesn't reflect the deployment environment. Unit tests are meant to be the first line of defence, catching mistakes early and giving developers confidence; as they exist today, these tests don't do that.

To be effective, the test suite should run in a TypeScript-aware environment, so that interface conformance is enforced during development and the tests provide meaningful feedback. One potential way to implement this would be using Jest + ts-jest, though other solutions that achieve the same result would also work. Jest and ts-jest are trivial to implement.

[onlywei]

I think we should just change the test script to:

"test": "tsc --noEmit && node --test"

This is exactly what ts-jest does under the hood, so I'd rather do this than add another tool.

[neilherbertuk]

Unfortunately, that doesn't work.

[onlywei]

Try #22

[neilherbertuk]

That works, much better :)