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

Tests demonstrating toError and fromError usage #37

Open
tegefaulkes opened this issue Oct 10, 2023 · 4 comments
Open

Tests demonstrating toError and fromError usage #37

tegefaulkes opened this issue Oct 10, 2023 · 4 comments
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Oct 10, 2023

Specification

There are tests testing the error serialization and deserailazation. But none of theses are using a passed in toError and fromError function.

We need to add tests demonstrating how to create a custom from/to error functions and demonstrate their usage as well as confirm that they are working as intended.

Additional context

Tasks

  1. add tests demonstrating the usage of toError and fromError that has been passed in as parameters to the RPCClient and RPCServer.
@tegefaulkes tegefaulkes added the development Standard development label Oct 10, 2023
@addievo
Copy link
Contributor

addievo commented Oct 10, 2023

  1. Currently only tests testing internal RPC errors exists. i.e
testProp(
    'RPC Serializes and Deserializes ErrorRPCRemote',
    [
      rpcTestUtils.safeJsonValueArb,
      rpcTestUtils.errorArb(rpcTestUtils.errorArb()),
    ],
    async (value, error) => {
      const { clientPair, serverPair } = rpcTestUtils.createTapPairs<
        Uint8Array,
        Uint8Array
      >();

      class TestMethod extends UnaryHandler {
        public handle = async (
          _input: JSONValue,
          _cancel: (reason?: any) => void,
          _meta: Record<string, JSONValue> | undefined,
          _ctx: ContextTimed,
        ): Promise<JSONValue> => {
          throw error;
        };
      }
      const rpcServer = await RPCServer.start({
        manifest: {
          testMethod: new TestMethod({}),
        },
        logger,
        idGen,
      });
      rpcServer.handleStream({ ...serverPair, cancel: () => {} });

      const rpcClient = await RPCClient.createRPCClient({
        manifest: {
          testMethod: new UnaryCaller(),
        },
        streamFactory: async () => {
          return { ...clientPair, cancel: () => {} };
        },
        logger,
        idGen,
      });

      const errorInstance = new ErrorRPCRemote(
        { code: -32006 },
        'Parse error',
        { cause: error },
      );

      const serializedError = fromError(errorInstance);
      const callProm = rpcClient.methods.testMethod(serializedError);
      await expect(callProm).rejects.toThrow(rpcErrors.ErrorRPCRemote);

      const deserializedError = toError(serializedError);

      expect(deserializedError).toBeInstanceOf(ErrorRPCRemote);

      // Check properties explicitly
      const { code, message, data } = deserializedError as ErrorRPCRemote<any>;
      expect(code).toBe(-32006);

      await rpcServer.stop({ force: true });
      await rpcClient.destroy();
    },
  );

And a similar test testing the replacer.

  1. However, we need to test application specific errors to validate toError and fromError for use in Polykey.

@addievo
Copy link
Contributor

addievo commented Oct 10, 2023

@tegefaulkes do you think it makes more sense to have the same test in Polykey, as we are testing Polykey specific errors, or should I make one in RPC creating a dummy abstract error?

@tegefaulkes
Copy link
Contributor Author

We need tests in rpc testing and demonstrating the usage of an application provided toError and fromError.

The above does not demonstrate this as it's using the js-rpc defaulted toError and fromError. Also does that test even work? it's not using the up to date API, RPCClient isn't CreateDestroy anymore.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 10, 2023

@tegefaulkes do you think it makes more sense to have the same test in Polykey, as we are testing Polykey specific errors, or should I make one in RPC creating a dummy abstract error?

Don't do anything right now. It's not a priority. But it does not make sense to do this test in PK. And here, it should only use regular errors (regular errors already support cause chains), it does not require a fully abstract error hierarchy to demonstrate everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity
Development

No branches or pull requests

3 participants