MatrixAI / Polykey

Polykey Core Library
https://polykey.com
GNU General Public License v3.0
31 stars 4 forks source link

Split tests for `client/handlers/vaults.test.ts` #839

Open aryanjassal opened 2 weeks ago

aryanjassal commented 2 weeks ago

Specification

Due to a resource leak crashing the CI, all the tests needed to be grouped. Right now, all the VaultsSecrets and Vaults RPC handlers are being tested in the same file. As a single file cannot be run concurrently, running the entire file is taking up to 3 minutes. It is also making the tests difficult to view/modify/debug.

As such, the tests need to be split into individual files as before. This will help speed up the CI too, as the tests will finish quicker.

Additional context

Tasks

  1. Split each describe block from client/handlers/vault.test.ts into its own file
  2. Resolve the resource leak which was crashing the CI
linear[bot] commented 2 weeks ago

ENG-456 Split tests for `client/handlers/vaults.test.ts`

aryanjassal commented 1 week ago

There are tests which test multiple domains at the same time. Specifically, the tests for vaultsSecretsNew, vaultsSecretsDelete, and vaultsSecretsGet are all in the same describe block, which makes ensuring full coverage of tests challenging.

These tests should be broken up into separate describe blocks. And instead of using the RPC to make/get/delete files, we can use the VaultOps methods to do so. Doing it this way will introduce more isolation and allow for better test writing.

aryanjassal commented 1 week ago

There are tests which test multiple domains at the same time. Specifically, the tests for vaultsSecretsNew, vaultsSecretsDelete, and vaultsSecretsGet are all in the same describe block, which makes ensuring full coverage of tests challenging.

These tests should be broken up into separate describe blocks. And instead of using the RPC to make/get/delete files, we can use the VaultOps methods to do so. Doing it this way will introduce more isolation and allow for better test writing.

This is being addressed in Polykey#838.

aryanjassal commented 1 week ago

While it would take longer, many RPC handlers like VaultsSecretsGet can be easily tested using fastcheck. This should be done when separating and cleaning up the tests.

CMCDragonkai commented 1 week ago

This PR is for splitting the tests... right? Generally splitting is good. You should find out WHY splitting is not possible and that is a code smell. So we don't want to needdlessly group tests that don't need to be grouped unless they are sharing a resource of some sort that you need to synchronize access to. But even then it's best that this is not an issue.