CLI and Client & Agent Service test splitting

[emmacasolin]

Description

This PR will split our CLI and GRPC API tests into individual files for each command/handler, as well as refactor the tests to improve performance and coverage.

Issues Fixed

• Fixes Split CLI and GRPC API tests #308
• Fixes Fix node claims throughout tests/ #306
• Fixes Propagate root keypair changes to domains external to KeyManager #312

Tasks

1. For all tests/bin, tests/client, and tests/agent:

• Split tests for each command/handler into their own individual files (except for commands that directly correspond to another command, e.g. identitiesTokenGet/identitiesTokenPut/identitiesTokenDelete)
• Have all tests use the global shared agent and/or the nested describe pattern used by tests/bin/agent/status.test.ts where appropriate
• Use module mocking for prompting
• Use the correct pkExec, pkStdio, pkSpawn, pkExpect utility
• Only use nested describe if there is a need to use own agent (rather than global agent)
• Revert all side effects in the global shared agent and the end of each test
• Remove all state setup/teardown from beforeEach and afterEach
• Make sure WARN is the default wherever LogLevel is used

9. Remove any unnecessary tests from GRPCClientClient.test.ts and GRPCClientAgent.test.ts
10. Check for other API conformance/design issues, such as error handling
11. Check order of stopping (in particular proxy stopping in notifications manager)
12. Replace all use of clearXXX() with destroy methods in tests
13. Restart method for agent for clearing side effects from the global agent that cannot be otherwise undone
14. Replace polykeyStartupTimeout with defaultTimeout
15. Fix up node claiming in tests (some tests set them up as singly-signed claims when they should be signed by both nodes) - from Fix node claims throughout tests/ #306
16. Integrate EventBus into PK to be used for asynchronously emitting root keypair changes
17. Fix up imports to only be using selective imports (except for '../utils')
18. Fix identities bin tests and gestalts client service tests

Tests that need splitting:

tests/bin (these should be fully simulated wherever possible using the global agent, then own agent using global keypair, then own agent fully created from scratch)

• agent
  • lock
  • lockall
  • start
  • status
  • stop
  • unlock
• identities
  • authenticate (must create own agent in order to undo authentication)
  • claim (must create own agent in order to unclaim)
  • search (must create own agent in order to register provider)
  • allow
  • disallow
  • discover
  • get
  • list
  • permissions
  • trust
  • untrust
• notifications
  • send
  • read
  • clear
• keys
  • cert
  • certchain
  • decrypt
  • encrypt
  • password
  • renew
  • reset
  • root
  • sign
  • verify
• nodes - ETA [Thur 13/1]
  • add (must create own agent to un-add + remote agent(s) to add)
  • claim (must create own agent to unclaim + remote agent to claim)
  • find (must create own and remote agent(s) for same reason as add)
  • ping (must create own and remote agent(s) for same reason as add)

tests/client (these can be mocked as much as possible and usually don't need a whole agent) - ETA [Wed 12/1]

• agentLockAll (only needs sessionManager)
• agentStatus (needs keyManager, grpcServerClient, grpcServerAgent, fwdProxy, revProxy)
• agentStop
• agentUnlock
• gestaltsActionsGetByIdentity (needs gestalt graph)
• gestaltsActionsGetByNode (needs gestalt graph)
• gestaltsActionsSetByIdentity (needs gestalt graph)
• gestaltsActionsSetByNode (needs gestalt graph)
• gestaltsActionsUnsetByIdentity (needs gestalt graph)
• gestaltsActionsUnsetByNode (needs gestalt graph)
• gestaltsDiscoveryByIdentity (needs discovery)
• gestaltsDiscoveryByNode (needs discovery)
• gestaltsGestaltGetByIdentity (needs gestalt graph)
• gestaltsGestaltGetByNode (needs gestalt graph)
• gestaltsGestaltList (needs gestalt graph)
• identitiesAuthenticate (needs identities manager)
• identitiesClaim (needs identitiesManager, sigchain, nodeManager - mock authentication + claiming process)
• identitiesInfoGet (needs identities manager)
• identitiesInfoGetConnected (needs identities manager)
• identitiesProvidersList (needs identities manager)
• identitiesTokenDelete (needs identities manager)
• identitiesTokenGet (needs identities manager)
• identitiesTokenPut (needs identities manager)
• keysCertsChainGet (needs keyManager)
• keysCertsGet (needs keyManager)
• keysDecrypt (needs keyManager)
• keysEncrypt (needs keyManager)
• keysKeyPairRenew (needs keyManager, nodeManager, fwdProxy, revProxy, grpcServerClient)
• keysKeyPairReset (needs keyManager, nodeManager, fwdProxy, revProxy, grpcServerClient)
• keysKeyPairRoot (needs keyManager)
• keysPasswordChange (needs keyManager)
• keysSign(needs keyManager)
• keysVerify (needs keyManager)
• nodesAdd (needs nodeManager)
• nodesClaim (needs nodeManager + notificationsManager - mock claiming process)
• nodesFind (needs nodeManager - mock connecting to other node)
• nodesPing (needs nodeManager - mock connecting to other node)
• notificationsClear (needs notificationsManager - mock receiving notifications)
• notificationsRead (needs notificationsManager - mock receiving notifications)
• notificationsSend (needs notificationsManager - mock receiving notifications + connecting to other node)

tests/agent (these should be fully simulated wherever possible) - ETA [Mon 17/1]

• nodesCrossSignClaim (must create own agent and agent to claim)
• notificationsSend (must create a second agent)

########################################################################

Tests that cannot yet be addressed:

Commands that may be removed

tests/bin/agent/restart

tests/client/service/agentRestart

Commands that will be implemented/fixed in #266

tests/bin/secrets

• env
• create
• delete
• dir
• edit
• get
• list
• mkdir
• update

tests/bin/vaults

• clone
• permissions
• pull
• scan
• share
• stat
• unshare
• create
• delete
• list
• log
• rename
• version

tests/client/service

• vaultsClone
• vaultsPermissions
• vaultsPermissionsSet
• vaultsPermissionsUnset
• vaultsPull
• vaultsScan
• vaultsSecretsStat
• vaultsCreate
• vaultsDelete
• vaultsList
• vaultsLog
• vaultsRename
• vaultsSecretsDelete
• vaultsSecretsEdit
• vaultsSecretsGet
• vaultsSecretsList
• vaultsSecretsMkdir
• vaultsSecretsNew
• vaultsSecretsNewDir
• vaultsSecretsRename
• vaultsVersion

tests/agent/service

• vaultsGitInfoGet
• vaultsGitPackGet
• vaultsPermissionsCheck
• vaultsScan

Tests that are yet to be written in #326

• nodesChainDataGet
• nodesClosestLocalNode
• nodesHolePunchMessage

Other unimplemented commands

tests/agent/service/nodesClaimsGet

Final checklist

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

[emmacasolin]

I've encountered a problem with the identities CLI - as far as I can see there isn't a command to add a node or identity to the gestalt graph, or any other command that would do this as a side-effect. Since the node/identity needs to be added to the graph before we can allow/disallow permissions (and probably other operations too), I can't see a way for these commands to work from a CLI perspective?

These tests were previously setting the nodes/identities manually (e.g. await polykeyAgent.gestaltGraph.setNode(node1)), however, this isn't possible with the shared agent. While I could probably mock the gestalt graph for testing purposes there's still the issue of these commands potentially not working for real users, unless I'm missing something.

[emmacasolin]

Related to my previous comment - there also aren't CLI commands for the identities token manipulation grpc handlers (i.e. identitiesTokenGet, identitiesTokenPut, identitiesTokenDelete), which means I don't think identities can be authenticated or claimed using solely the CLI.

[CMCDragonkai]

I believe that you can definitely authenticate via the CLI. But it involves using the browser.

@joshuakarp had previously mocked the TestProvider/browser call so that it was fully automated in the tests.

All other identities related functionality would require mocking and enhancing the flexibility of the TestProvider. Right now I'm not sure if the TestProvider is applied on the global shared agent. This was something we had discussed but didn't get around to.

In which case I suggest you focus on the other test areas to first split. We can then circle back to the TestProvider problem and mock it's behaviour.

And yes there should probably be more identities CLI calls, we hadn't fully reviewed the workflow here yet.

[CMCDragonkai]

Just a note about the final describe block that exists for bin tests that use the global agent. Make sure to name the describe like this:

  describe('status with global agent', () => {

Replace the status with the relevant command, and this section should always be at the bottom.

[emmacasolin]

Decided to move onto the GRPC tests since there are a lot of things blocking the CLI tests (need to look into mocking the identities manager + gestalt graph and/or adding more CLI calls to fill gaps), however there are errors in tests/client/service/agentStop.test.ts that prevent it from compiling so I'm hesitant to use it as a base before this is resolved. All of the errors are the same:

tests/client/service/agentStop.test.ts:103:20 - error TS2339: Property 'running' does not exist on type 'PolykeyAgent'.

    103     expect(pkAgent.running).toBe(false);
                           ~~~~~~~

Not sure what change between when the file was created 9 days ago and now has caused this issue.

[emmacasolin]

The tests that have been split up so far and that are passing are:

• keys/cert.test.ts
• keys/certchain.test.ts
• keys/decrypt.test.ts
• keys/encrypt.test.ts
• keys/password.test.ts
• keys/renew.test.ts

I've also done identities/allow.test.ts, identities/authenticate.test.ts, and started on identities/claim.test.ts and notifications/send.test.ts, but all of these won't be able to be completed until we're able to mock the identities manager (test provider) and the gestalt graph. Similarly, I'm expecting the nodes domain will be difficult as well due to the side effects that can't be cleared, unless these tests use their own individual agents rather than the shared one.

[CMCDragonkai]

All uses of the global shared agent is serialised with mutual exclusion. So any test with side-effects just needs to reverse those side-effects at the end. As long as the ability to reverse is available, then testing should be possible.

[emmacasolin]

The keys CLI tests have all been split into their own files. I was having issues with keys reset where the agent was needing to be stopped and restarted after calling keys reset in order for subsequent commands to work, so this test creates it's own agent process rather than using the shared agent. It also needed a timeout due to this, as it can take a while to run, particularly when run with the rest of the keys tests.

[CMCDragonkai]

@emmacasolin when you get to the TestProvider mocking stage. Consider that there are 3 ways of doing mocking.

1. Mocking a single function in a module
2. Mocking an entire module
3. Mocking a single method inside an class/object instance

We have used all 3 mocking patterns in other test areas. Consult with @tegefaulkes to get an idea of how to do it.

I reckon we would need to mock the IdentitiesManager in the global shared agent. This can be done because the global agent is using pkStdio. Since it currently registers the GitHubProvider, all we may need to do is just pre-register it on our mock prior to running pkStdio. You'll have to prototype the different ways of doing it, perhaps by creating the mock instance and then just registering the provider and expect that inside the pkStdio it would acquire the same instance... Or by mocking out the constructor of IdentitiesManager or createIdentitiesManager static method and add it there. This mock wouldn't need to be cleared because it's long running and global in the agent.

[CMCDragonkai]

Btw if you find places where side effects can't be reversed (and no, not with a clear method), those places should add the opposite method to keep their APIs symmetric.

However I know that say Sigchain may not be capable of doing this. At least not for a single claim. But then a general reset of the sigchain can be exposed to the client service that allows complete wipe.

[CMCDragonkai]

When this is ready, feel free to merge back into qa-testing.

[CMCDragonkai]

Decided to move onto the GRPC tests since there are a lot of things blocking the CLI tests (need to look into mocking the identities manager + gestalt graph and/or adding more CLI calls to fill gaps), however there are errors in tests/client/service/agentStop.test.ts that prevent it from compiling so I'm hesitant to use it as a base before this is resolved. All of the errors are the same:

tests/client/service/agentStop.test.ts:103:20 - error TS2339: Property 'running' does not exist on type 'PolykeyAgent'.

    103     expect(pkAgent.running).toBe(false);
                           ~~~~~~~

Not sure what change between when the file was created 9 days ago and now has caused this issue.

To resolve this. Go to src/PolykeyAgent.ts and find the statement pkAgent[running].

Replace test expectations using pkAgent.running with pkAgent[running].

Import the symbol running from async-init.

[CMCDragonkai]

The keys CLI tests have all been split into their own files. I was having issues with keys reset where the agent was needing to be stopped and restarted after calling keys reset in order for subsequent commands to work, so this test creates it's own agent process rather than using the shared agent. It also needed a timeout due to this, as it can take a while to run, particularly when run with the rest of the keys tests.

Why do we need to restart the agent when calling keys reset? It should work without doing this. If it requires a restart, that would be a bug.

[emmacasolin]

Why do we need to restart the agent when calling keys reset? It should work without doing this. If it requires a restart, that would be a bug.

In that case I'll play around with it and see what's going on. It could just be an issue with timing, not sure.

[emmacasolin]

The error that's occurring on commands that follow keys reset is coming from verifyServerCertificateChain() in the network utils. After the root keypair is reset the root certificate chain is also reset, so the certificate claiming the nodeId used to contact the server is removed. This means that the client can't be authenticated to the server. I think the reason that stopping and re-starting the agent removes this issue is that this allows the nodeId to be updated? Otherwise the old nodeId is used, which can no longer be verified against the updated tls config.

[CMCDragonkai]

Ah yes so basically when this happens the new node ID should be used by the client GRPCClient.

Or if this is between agent to agent, then the new TLSConfig must be set for the Forward and Reverse proxies.

This was something we had to a while ago for agent to agent networking and we provisioned for this instance.

Look at the handler for key reset and the associated KeyManager method, see how it's supposed to rencrypt all the keys and reset the TLSConfig. The whole process is meant to be a graceful swap.

[emmacasolin]

The problem with keys reset in the end came down to the status file not being updated when the root keypair is changed, so I injected the Status into KeyManager and added a method to Status to allow the node id to be updated, and called this in both resetRootKeypair() and renewRootKeypair(). Now the agent doesn't need to be restarted in order to write the new node id into the status file, since previously this was the only way of writing anything to the file.

[emmacasolin]

During my meeting today with @tegefaulkes, we decided that the simplest way to "mock" the Test Provider usage was to just register it during the setup of the global agent in tests/utils.ts. As @CMCDragonkai mentioned, registering the test provider is a long-running, global operation and doesn't need to be cleared, so registering it during setup seems like the best way to go about it.

[CMCDragonkai]

Yea I think that's a good idea. But the question is how to do it, whether by module mocking or class method mocking? Do we use the jest.mock or jest.spyOn?

[CMCDragonkai]

The problem with keys reset in the end came down to the status file not being updated when the root keypair is changed, so I injected the Status into KeyManager and added a method to Status to allow the node id to be updated, and called this in both resetRootKeypair() and renewRootKeypair(). Now the agent doesn't need to be restarted in order to write the new node id into the status file, since previously this was the only way of writing anything to the file.

I'm thinking that injecting status like this isn't the best idea and instead composition should be used. But I need to think how to propagate changes of keypair changes to all the places that need to be updated when the keypair changes.

[CMCDragonkai]

Example: https://www.sitepoint.com/three-design-patterns-that-use-inversion-of-control/

[emmacasolin]

Yea I think that's a good idea. But the question is how to do it, whether by module mocking or class method mocking? Do we use the jest.mock or jest.spyOn?

Just simply calling the registerProvider() function as usual:

    const pkAgent = await PolykeyAgent.createPolykeyAgent({
      password: globalAgentPassword,
      nodePath: globalAgentDir,
      keysConfig: {
        rootKeyPairBits: 1024
      },
      seedNodes: {}, // explicitly no seed nodes on startup
      logger,
    });
    pkAgent.identitiesManager.registerProvider(new TestProvider());

This has already been added to tests/utils.ts (lines 112-121) and the authenticate test is passing with this method of doing it. We're actually registering the test provider so no mocking is needed.

[CMCDragonkai]

But the global shared agent doesn't give you access to the PK agent.

On 30 December 2021 7:35:21 pm AEDT, emmacasolin ***@***.***> wrote: > Yea I think that's a good idea. But the question is how to do it, whether by module mocking or class method mocking? Do we use the jest.mock or jest.spyOn? Just simply calling the `registerProvider()` function as usual: ```ts const pkAgent = await PolykeyAgent.createPolykeyAgent({ password: globalAgentPassword, nodePath: globalAgentDir, keysConfig: { rootKeyPairBits: 1024 }, seedNodes: {}, // explicitly no seed nodes on startup logger, }); pkAgent.identitiesManager.registerProvider(new TestProvider()); ``` This has already been added to `tests/utils.ts` (lines 112-121) and the `authenticate` test is passing with this method of doing it. We're actually registering the test provider so no mocking is needed. -- Reply to this email directly or view it on GitHub: #311 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[emmacasolin]

But the global shared agent doesn't give you access to the PK agent.

We don't need to access it - the registration is being done inside the setupGlobalAgent function

[CMCDragonkai]

Yep I forgot it was changed over to like that. So that should be sufficient, you can proceed with that solution.

[emmacasolin]

I've implemented the observer pattern to enable places that need to be updated when the keypair is changed to do so automatically. The relevant tests are passing, but the naming I've used isn't great so I want to fix that at some point. At the moment only NodeManager and Status are "observers" of the KeyManager - is there anything else that should be an observer as well? It's easy enough to change this later on by simply having whatever class needs to monitor the keypair implement the ObserverKeypair interface in src/keys/types.ts.

[emmacasolin]

The remaining instances of 0.0.0.0 appearing in test outputs are the following tests. The ports are likely random for the most part, however I included them to show how many occurances there were for the same port in each test.

tests/nodes/NodeConnectionManager.general.test.ts

• WARN:ConnectionForward 0.0.0.0:58343:Forward Error: ErrorConnectionEndTimeout (x2)
• WARN:ConnectionForward 0.0.0.0:43971:Forward Error: ErrorConnectionEndTimeout (x2)

tests/nodes/NodeConnectionManager.lifecycle.test.ts

• WARN:ConnectionForward 0.0.0.0:46710:Forward Error: ErrorConnectionEndTimeout (x7)
• WARN:ConnectionForward 0.0.0.0:56352:Forward Error: ErrorConnectionEndTimeout (x1)

tests/nodes/NodeConnectionManager.seednodes.test.ts

• WARN:ConnectionForward 0.0.0.0:51387:Forward Error: ErrorConnectionEndTimeout (x2)

tests/nodes/NodeConnectionManager.termination.test.ts

• WARN:ConnectionForward 0.0.0.0:46311:Forward Error: ErrorConnectionEndTimeout (x1)
• WARN:ConnectionForward 0.0.0.0:52191:Forward Error: ErrorConnectionEndTimeout (x1)
• WARN:ConnectionForward 0.0.0.0:39933:Forward Error: ErrorConnectionEndTimeout (x1)
• WARN:ConnectionForward 0.0.0.0:33209:Forward Error: ErrorConnectionEndTimeout (x1)
• WARN:ConnectionForward 0.0.0.0:54328:Forward Error: ErrorConnectionEndTimeout (x1)
• WARN:ConnectionForward 0.0.0.0:55269:Forward Error: ErrorConnectionEndTimeout (x1)
• WARN:ConnectionForward 0.0.0.0:52081:Forward Error: ErrorConnectionEndTimeout (x1)
• WARN:ConnectionForward 0.0.0.0:44664:Forward Error: ErrorConnectionEndTimeout (x1)

tests/nodes/NodeConnectionManager.termination.test.ts

• WARN:ConnectionForward 0.0.0.0:59681:Forward Error: ErrorConnectionEndTimeout (x3)

While all of these warnings are for the same error, there are other occurances of the same warning where the host takes the correct value of 127.0.0.1, for example WARN:ConnectionForward 127.0.0.1:37133:Forward Error: ErrorConnectionEndTimeout which comes from tests/nodes/NodeConnection.test.ts.

[CMCDragonkai]

I've checked the final npm run build. Proceeding to merge.

The final nodes related issues above will be addressed in #326.