Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aryanjassal opened this issue Nov 1, 2024 · 5 comments
Open

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

aryanjassal opened this issue Nov 1, 2024 · 5 comments
Labels
development Standard development

Comments

@aryanjassal
Copy link
Contributor

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
@aryanjassal aryanjassal added the development Standard development label Nov 1, 2024
Copy link

linear bot commented Nov 1, 2024

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
Development

No branches or pull requests

2 participants