Standardise process execution for tests

[emmacasolin]

Description

There are many ways of running our tests across different platforms, and this should be standardised.

• Our pkExec/pkSpawn/pkExpect methods should have a default and override implementation. Default should run polykey as normal while the override should run polykey from a different entrypoint. The override should be taken from an argument first and then from an environment variable.
• The remaining methods should not have override implementations, as these are meant for special cases (e.g. mocking for pkStdio and general process execution for exec). All places in the code where such methods are used and these special cases do not apply should switch over to using pkExec or similar.
• The override methods should be exported as well as the defaults to be used for edge cases.

We will need to ensure that signals and events are still handled correctly after making these changes.

Issues Fixed

• Fixes Standardise Process Execution in tests/utils/exec.ts #432

Tasks

• 1. Move test/utils.ts into tests/utils/utils.ts and create tests/utils/index.ts
• 2. Change the pkExec, pkSpawn, pkExpect signatures to taken an options object at the very end for the command, env, and cwd
• 3. Refactor pkExec, pkSpawn, pkExpect according to spec in Standardise Process Execution in tests/utils/exec.ts #432
• 4. Remove pkExecNs and pkSpawnNS and replace with pkExec and pkSpawn with command override options
• 5. Incorporate https://eslint.org/docs/latest/rules/no-restricted-globals

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[ghost]

👇 Click on the image for a new way to code review

• Make big changes easier — review code in small groups of related files

• Know where to start — see the whole change at a glance

• Take a code tour — explore the change with an interactive tour

• Make comments and review — all fully sync’ed with github

  Try it now!

Legend

[emmacasolin]

Aside from converting pkExpect to use the same style as pkExec and pkSpawn (#432 (comment)), this should be ready for CI/CD redeployment and testing with docker.

[CMCDragonkai]

Regarding 501b803, this should be a separate PR, this requires changes to:

1. All docker tests
2. Polykey-Infrastructure - this requires a new PR in the gitlab repo
3. README instructions on container/docker usage

[CMCDragonkai]

@tegefaulkes

Is fd87744 going to be rebased with #437?

[emmacasolin]

Ah ok. I've dropped that commit and updated the PR description. So this is now just for standardising the process execution utils.

[CMCDragonkai]

This requires rebase on staging. The globalThis is being used now. Don't use window nor global.

[emmacasolin]

I can't find any mention of there being a shell option for nexpect.spawn, and trying to add it in results in a TypeError. This is the definition for the options object that it expects:

interface ISpawnOptions {
    cwd?: string | undefined;
    env?: any;
    ignoreCase?: any;
    stripColors?: any;
    stream?: any;
    verbose?: any;
}

---

As a side note, the nexpect library doesn't actually use child_process.spawn in the latest release, and instead uses cross_spawn.spawn. We're still using version 0.4.2, but this means we can't update nexpect if we specifically care about using child_process underneath.

---

Interestingly it seems they made the switch due to compatibility issues on Windows nodejitsu/nexpect#37

[emmacasolin]

I can't find any mention of there being a shell option for nexpect.spawn, and trying to add it in results in a TypeError. This is the definition for the options object that it expects:

interface ISpawnOptions {
    cwd?: string | undefined;
    env?: any;
    ignoreCase?: any;
    stripColors?: any;
    stream?: any;
    verbose?: any;
}

As a side note, the nexpect library doesn't actually use child_process.spawn in the latest release, and instead uses cross_spawn.spawn. We're still using version 0.4.2, but this means we can't update nexpect if we specifically care about using child_process underneath.

Interestingly it seems they made the switch due to compatibility issues on Windows nodejitsu/nexpect#37

Regardless of whether nexpect uses child_process or cross_spawn (which just wraps child_process and passes the options through), the problem is that nexpect doesn't simply pass the options as is to the underlying spawn call. Rather the options are injected into a different context object that then gets passed into all of the underlying methods.

  options = options || {};
  context = {
    command: command,
    cwd: options.cwd || undefined,
    env: options.env || undefined,
    ignoreCase: options.ignoreCase,
    params: params,
    queue: [],
    stream: options.stream || 'stdout',
    stripColors: options.stripColors,
    verbose: options.verbose
  };

So I don't think it's possible for pkExpect to have a shell: true override if we're using nexpect. Since we aren't using pkExpect at the moment, and since this is blocking #439, I think converting pkExpect can go into a separate issue and be put on hold for now. Everything else for this PR is done, just need to squash down the commits.

[tegefaulkes]

Just 2 comments for now. I need to head off for now but I'll come back and do a more thorough review.

[CMCDragonkai]

Ok pkExpect can be done in a later issue.

[CMCDragonkai]

Add a comment to pkExpect to indicate this.

[CMCDragonkai]

I think the setupTestAgent doesn't belong in tests/utils/exec.ts. That isn't about executing some arbitrary command, but is more like a fixture itself.

You can keep it in tests/utils/utils.ts for now.

Plus everywhere should be using import testUtils from '../utils';. And the index.ts already exports (un-namespaced) all the utility functions anyway.

If it doesn't exist, please create tests/utils/index.ts that rexports all the relevant functions:

export * from './utils.ts';
export * from './exec.ts';

This is similar to src/utils/index.ts.

Any tests/utils.ts should be now be tests/utils/utils.ts.

[CMCDragonkai]

I can't find any mention of there being a shell option for nexpect.spawn, and trying to add it in results in a TypeError. This is the definition for the options object that it expects:

interface ISpawnOptions {
    cwd?: string | undefined;
    env?: any;
    ignoreCase?: any;
    stripColors?: any;
    stream?: any;
    verbose?: any;
}

As a side note, the nexpect library doesn't actually use child_process.spawn in the latest release, and instead uses cross_spawn.spawn. We're still using version 0.4.2, but this means we can't update nexpect if we specifically care about using child_process underneath.
Interestingly it seems they made the switch due to compatibility issues on Windows nodejitsu/nexpect#37

Regardless of whether nexpect uses child_process or cross_spawn (which just wraps child_process and passes the options through), the problem is that nexpect doesn't simply pass the options as is to the underlying spawn call. Rather the options are injected into a different context object that then gets passed into all of the underlying methods.

  options = options || {};
  context = {
    command: command,
    cwd: options.cwd || undefined,
    env: options.env || undefined,
    ignoreCase: options.ignoreCase,
    params: params,
    queue: [],
    stream: options.stream || 'stdout',
    stripColors: options.stripColors,
    verbose: options.verbose
  };

So I don't think it's possible for pkExpect to have a shell: true override if we're using nexpect. Since we aren't using pkExpect at the moment, and since this is blocking #439, I think converting pkExpect can go into a separate issue and be put on hold for now. Everything else for this PR is done, just need to squash down the commits.

Did you create a separate issue for this?

[emmacasolin]

I can't find any mention of there being a shell option for nexpect.spawn, and trying to add it in results in a TypeError. This is the definition for the options object that it expects:

interface ISpawnOptions {
    cwd?: string | undefined;
    env?: any;
    ignoreCase?: any;
    stripColors?: any;
    stream?: any;
    verbose?: any;
}

As a side note, the nexpect library doesn't actually use child_process.spawn in the latest release, and instead uses cross_spawn.spawn. We're still using version 0.4.2, but this means we can't update nexpect if we specifically care about using child_process underneath.
Interestingly it seems they made the switch due to compatibility issues on Windows nodejitsu/nexpect#37

Regardless of whether nexpect uses child_process or cross_spawn (which just wraps child_process and passes the options through), the problem is that nexpect doesn't simply pass the options as is to the underlying spawn call. Rather the options are injected into a different context object that then gets passed into all of the underlying methods.

  options = options || {};
  context = {
    command: command,
    cwd: options.cwd || undefined,
    env: options.env || undefined,
    ignoreCase: options.ignoreCase,
    params: params,
    queue: [],
    stream: options.stream || 'stdout',
    stripColors: options.stripColors,
    verbose: options.verbose
  };

So I don't think it's possible for pkExpect to have a shell: true override if we're using nexpect. Since we aren't using pkExpect at the moment, and since this is blocking #439, I think converting pkExpect can go into a separate issue and be put on hold for now. Everything else for this PR is done, just need to squash down the commits.

Did you create a separate issue for this?

Not yet. Will reference here once I have.

[CMCDragonkai]

There's also:

nat/utils.ts
1:import type { ChildProcess } from 'child_process';
5:import child_process from 'child_process';
134:  const subprocess = child_process.spawn(
159:  const subprocess = child_process.spawn(

Should these be using our exec utilities?

[emmacasolin]

There's also:

nat/utils.ts
1:import type { ChildProcess } from 'child_process';
5:import child_process from 'child_process';
134:  const subprocess = child_process.spawn(
159:  const subprocess = child_process.spawn(

Should these be using our exec utilities?

Yeah it should be possible for those to switch over to using the new exec. I'll make that change.

[CMCDragonkai]

Beware that the new exec only executes right, whereas you have the subprocess there. If you need the subprocess, you may need to keep the childProcess.spawn.

[CMCDragonkai]

Please add this in the end to ignore the src/proto from coverage reports: https://jestjs.io/docs/configuration#coveragepathignorepatterns-arraystring

[emmacasolin]

The nodes tests are timing out locally as well. I'm not sure what caused them to suddenly start doing this - the only change between the last time they were passing and the first time they started failing was changing the imports. I tried reverting this for NodeConnection.test.ts (where the timeouts are) but this didn't fix the issue.

[CMCDragonkai]

Have a look at NodeConnection.test.ts. It's just acquiring the test process as a dummy process for GRPC. If your testUtils.spawn is simple enough, then it should work fine. You can swap it over and remove spawnFile.

Obviously you want to look into why nodes tests are timing out now. Run them individually as well. Maybe see if this was brought in by some previous commits @tegefaulkes any ideas?

[tegefaulkes]

Node connection tests will be using agents to test against. Check that all of the agents have the key generation override applied to them. It should look like privateKeyPemOverride: globalRootKeyPems[2],, check the key managers too.

If not that then are some tests timeout a connection when they shouldn't be?

NodeConnection.test.ts are passing locally in the feature-concurrent-state branch. All the tests individually complete within 5 seconds.

[CMCDragonkai]

These 2 locations:

»» ~/Projects/Polykey/tests
 ♖ ag 'spawnFile'                                                         (staging) pts/4 16:04:44
utils/exec.ts
631:function spawnFile(path: string) {
795:  spawnFile,

nodes/NodeConnection.test.ts
41:import { spawnFile } from '../utils/exec';
738:        const testProcess = spawnFile('tests/grpc/utils/testServer.ts');
804:        const testProcess = spawnFile('tests/grpc/utils/testServer.ts')

Should be using ts-node directly. The files don't have a shebang and they don't have executable permissions. They are just scripts that must be executed with ts-node. Windows doesn't support shebangs.

Furthermore the path to the file is not robust. It should be using globalThis.testDir.

const testProcess = testUtils.spawn('ts-node', [ `${globalThis.testDir}/grpc/utils/testServer.ts` ]);

Also @tegefaulkes has mentioned that there is no error event handler for these functions. You need to attach an error event handler for the testUtils.spawn and testUtils.exec. So that when await testUtils.spawn it should be throwing an exception if an error event is thrown... (this is only relevant at startup).

[CMCDragonkai]

Ok so I think the solution is this.

Both testUtils.exec and testUtils.spawn use childProcess.spawn.

Now in the case of exec, there are 2 events you are listening on:

1. subprocess.on('error') - here you reject the Promise<{...}>
2. subprocess.on('exit') - here you resolve the Promise<{...}>

In the case of spawn, there are also 2 events you need to listen on:

1. subprocess.on('error') - here you reject the Promise<ChildProcess>
2. subprocess.on('spawn') - here you resolve Promise<ChildProcess> but you actually need to deregister the error event handler, because that event may still be emitted under other conditions

This way you can do await testUtils.spawn() and await testUtils.exec() and you can get an exception if the execution of the command fails.

See: https://nodejs.org/api/child_process.html#event-spawn

[CMCDragonkai]

If you're finishing up for the day @emmacasolin, @tegefaulkes can take over this, and get this merged.

[CMCDragonkai]

Note that this means the NodeConnection.test.ts should be changing to using await testUtils.spawn(...) in order to get a proper exception telling us why grpc/utils/testServer.ts is failing to be spawned.

Individual tests should still be listening on the error event in case the spawned process itself still results in some kind of error. The NodeConnection.test.ts is pretty legacy now, and I wish to refactor that entirely after #329 is resolved.

[CMCDragonkai]

When shell option is specified, the above behaviour applies to the shell process itself. But that's a caveat that has to be understood by the tester when using the shell option.

[tegefaulkes]

So, the command is not actually failing to spawn but it is throwing an error in stderr. The real problem is this snippet in tests/utils/exec.ts:21.

const polykeyPath = path.resolve(
  path.join(globalThis.projectDir, 'src/bin/polykey.ts'),
);

This is sneaky one. This used to be inline in each function and was only run when the function was called. But here it's run during import of tests/utils/exec.ts. This combined with the fact that globalThis.projectDir is only defined during jests tests was causing a type error when the testserver was run as a subprocess.

Error: thrown: "TypeError [ERR_INVALID_ARG_TYPE]: The \"path\" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:372:5)
    at validateString (node:internal/validators:120:11)
    at Object.join (node:path:1172:7)
    at Object.<anonymous> (/home/faulkes/matrixCode/polykey/js-polykey/tests/utils/exec.ts:22:8)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Module.m._compile (/home/faulkes/matrixCode/polykey/js-polykey/node_modules/ts-node/src/index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Object.require.extensions.<computed> [as .ts] (/home/faulkes/matrixCode/polykey/js-polykey/node_modules/ts-node/src/index.ts:1621:12)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12) {
  code: 'ERR_INVALID_ARG_TYPE'
}
"

[CMCDragonkai]

So I was wrong about the -p tsconfigPath not being needed. It is needed because the CWD is being changed when we run the commands. It's just the -r isn't needed anymore.

So everywhere ts-node is used, the --project ${tsconfigPath} should be used.

A similar const can be used like const tsconfigPath. But the same fix to globalThis.projectDir would need to be used.

Also NodeConnection.test.ts is directly using ts-node, that also requires the --project option too.