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

Centralising the Testing of WorkerManager Integration #271

Closed
CMCDragonkai opened this issue Oct 26, 2021 · 2 comments
Closed

Centralising the Testing of WorkerManager Integration #271

CMCDragonkai opened this issue Oct 26, 2021 · 2 comments
Labels
development Standard development

Comments

@CMCDragonkai
Copy link
Member

Specification

The WorkerManager is used by the keys and vaults domain. It is also additionally used by the DB imported by js-db.

The WorkerManager allows the use of multiple cores to aid in computationally-expensive operations. Right now, these operations are all crypto operations.

The usage of WorkerManager is made possible through dynamic setters and unsetters. It is not done through constructor injection unlike most other dependencies. This is because the WorkerManager can be engaged and un-engaged whenever. The only places where a constructor injection is used is in PolykeyAgent which bootstraps the whole architecture, and there a null can explicitly set to prevent the default usage of WorkerManager.

The testing for the WorkerManager involves running a worker pool for the relevant domains keys, vaults, PolykeyAgent, and testing their normal functionality while the WorkerManager is engaged. By default this would launch a worker pool with all the cores. This is quite expensive for testing. Instead, we need to ensure that all WorkerManager creations are created with only 1 core to make the pools small to create.

From a development perspective, these test should be addressable independently of non-WorkerManager tests. See problems in #264. The idea is that for example tests/keys/KeyManager.test.ts worker manager related tests can be separated to tests/keys/KeyManagerWorkers.test.ts.

There's no way to currently share a WorkerManager instance pool with multiple test files. But this can be done if all the relevant tests were combined into a single test file and then using describe nesting.

Another problem is that devs often forget to test the usage of WorkerManager for their domains. It should be done to ensure that parallel computation still works.

We need to work out a solution that helps solve #264 and ensures that future devs are always writing good tests for the usage of WorkerManager.

Additional context

Tasks

  1. ...
  2. ...
  3. ...
@CMCDragonkai CMCDragonkai added the development Standard development label Oct 26, 2021
@CMCDragonkai
Copy link
Member Author

By default the PolykeyAgent doesn't have workermanager and it's a setter injection now.

So pk agent start will plug in the worker manager there, but a flag can be used to not use multiple cores/workers. This can help us in our tests.

But as part of overall test optimisation many things should be moved to beforeAll and afterAll too.

So basically we don't need to test worker integration at the top level, except for a small sanity check. We can expect the workers work in our unit tests that allow worker integration. This should focus in all the tests that use workers.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jan 9, 2022

This is not a problem anymore because of #264 (comment)

Here is one of the exceptions where a nested describe can be useful.

Again creating a worker manager with 1 core isn't that expensive atm.

Going to close this for now as these are what can be done:

  1. Just use core count of 1 and see how long it takes.
  2. If you really need to, use a nested describe, but make sure it's put at the very end similar to how we do with the global agent in our tests/bin.
  3. Note that worker tests cannot be done in tests/bin atm, because of some problem with threadsjs. So right now worker tests are always domain-specific tests, and no integration testing can be done with workers yet.

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

1 participant