chore(errors): add custom error classes #508

jriffs · 2023-06-19T17:46:26Z

Description

Added custom error classes GRPCNotSupportedError and HTTPNotSupportedError and updated the following classes to use the new custom errors
-src/implementation/Client/HTTPClient/configuration.ts
-src/implementation/Client/HTTPClient/proxy.ts
-src/implementation/Server/GRPCServer/actor.ts

#297

Signed-off-by: jriffs <mayjriffs97@gmail.com>

src/version.ts

shubham1172 · 2023-06-19T18:22:56Z

src/implementation/Server/GRPCServer/actor.ts

  }

  registerActor<T extends AbstractActor>(_cls: Class<T>): Promise<void> {
-    throw new Error("GRPC is currently not supported.");


There might be more such errors now introduced in some PRs like #485

I've searched the repo and there doesn't seem to be anymore files with those errors or similar errors

One such example is https://github.com/shubham1172/js-sdk/blob/main/src/implementation/Client/GRPCClient/workflow.ts

okay, I have found and updated it.
Quick question0- Do i create a new folder errors in the test directory to place the unit test files or do i just add them to an existing folder in the same test directory

They should be a part of https://github.com/dapr/js-sdk/tree/main/test/unit/<an-existing-folder> to test if components return the correct error, and also in https://github.com/dapr/js-sdk/tree/main/test/unit/errors to test the error itself.

Signed-off-by: jriffs <mayjriffs97@gmail.com>

codecov · 2023-06-20T04:07:19Z

Codecov Report

Merging #508 (f365bbd) into main (0d16244) will decrease coverage by 0.47%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main     #508      +/-   ##
==========================================
- Coverage   35.69%   35.22%   -0.47%     
==========================================
  Files          87       90       +3     
  Lines       10125    10142      +17     
  Branches      414      415       +1     
==========================================
- Hits         3614     3573      -41     
- Misses       6452     6503      +51     
- Partials       59       66       +7

Impacted Files	Coverage Δ
src/implementation/Client/GRPCClient/workflow.ts	`30.00% <12.50%> (+7.77%)`	⬆️
src/implementation/Client/HTTPClient/workflow.ts	`12.69% <14.28%> (+1.40%)`	⬆️
.../implementation/Client/HTTPClient/configuration.ts	`42.85% <20.00%> (+9.52%)`	⬆️
src/implementation/Server/GRPCServer/actor.ts	`42.85% <20.00%> (+9.52%)`	⬆️
src/errors/GRPCNotSupportedError.ts	`33.33% <33.33%> (ø)`
src/errors/HTTPNotSupportedError.ts	`33.33% <33.33%> (ø)`
src/implementation/Client/HTTPClient/proxy.ts	`75.00% <50.00%> (+8.33%)`	⬆️
src/errors/PropertyRequiredError.ts	`100.00% <100.00%> (ø)`

... and 6 files with indirect coverage changes

shubham1172 · 2023-06-21T07:38:13Z

src/version.ts

This file has appeared again, I wonder is your IDE doing some magic 🤔

@shubham1172 I think it is doing some magic lol 😅, I suspect it does this when i run npm run pretty-fix command, but I'll be on the lookout next time. Could you please 🙏 give me a few pointers on how to resolve the code coverage issue, how do i test that each component returns the right error.

@shubham1172 still waiting on your response sir 😇

Hi @jriffs, sorry this got lost in my GitHub notifications, thanks for pinging. For code coverage, I think it is okay to skip testing "GRPCNotSupportedError" or "HTTPNotSupportedError" since we usually do not write tests for unimplemented code anyway.

For PropertyRequiredError, we can create unit tests under test/unit/http, for e.g. workflow.ts and with a similar pattern like HttpServerImpl.test.ts, we can expect methods to throw a certain error.

shubham1172 · 2023-07-03T14:22:51Z

src/implementation/Client/GRPCClient/workflow.ts

@@ -23,7 +24,7 @@ export default class GRPCClientWorkflow implements IClientWorkflow {
  }

  get(_instanceId: string, _workflowComponent?: string | undefined): Promise<WorkflowGetResponseType> {
-    throw new Error("Method not implemented.");
+    throw new MethodNotImplementedError();


This can be same as GrpcNotSupportedError, I don't think we will need MethodNotImplemented error then.

shubham1172 · 2023-07-03T14:28:01Z

src/version.ts

Hi @jriffs, sorry this got lost in my GitHub notifications, thanks for pinging. For code coverage, I think it is okay to skip testing "GRPCNotSupportedError" or "HTTPNotSupportedError" since we usually do not write tests for unimplemented code anyway.

For PropertyRequiredError, we can create unit tests under test/unit/http, for e.g. workflow.ts and with a similar pattern like HttpServerImpl.test.ts, we can expect methods to throw a certain error.

Signed-off-by: jriffs <mayjriffs97@gmail.com>

jriffs · 2023-07-10T15:40:37Z

Hi @shubham1172 , i just wanted to ask if there were any problems with the PR, if there are none, i was wondering if you could maybe assign me to a new issue so i can get working on that.
Thank you.

shubham1172 · 2023-07-10T16:20:44Z

Thanks @jriffs, leaving it open for some time to get more eyes. Meanwhile you could pick up the good first issue from the next milestone https://github.com/dapr/js-sdk/milestone/13 :)

fixing errors from earlier pull request

f621628

Signed-off-by: jriffs <mayjriffs97@gmail.com>

jriffs requested review from a team as code owners June 19, 2023 17:46

jriffs mentioned this pull request Jun 19, 2023

added custom error classes #497

Closed

shubham1172 suggested changes Jun 19, 2023

View reviewed changes

removed unwanted file �ersion.ts??

af421e3

Signed-off-by: jriffs <mayjriffs97@gmail.com>

Merge remote-tracking branch 'upstream/main' into jriffs/custom-errors

c99ed17

jriffs force-pushed the jriffs/custom-errors branch from cfd3823 to de37df0 Compare June 20, 2023 19:50

shubham1172 reviewed Jun 21, 2023

View reviewed changes

shubham1172 suggested changes Jul 3, 2023

View reviewed changes

created more custom error classes and added test suites for them

f365bbd

Signed-off-by: jriffs <mayjriffs97@gmail.com>

jriffs force-pushed the jriffs/custom-errors branch from de37df0 to f365bbd Compare July 4, 2023 23:11

shubham1172 approved these changes Jul 5, 2023

View reviewed changes

DeepanshuA approved these changes Jul 11, 2023

View reviewed changes

shubham1172 changed the title ~~fixing errors from earlier pull request~~ chore(errors): add custom error classes Jul 11, 2023

shubham1172 added this pull request to the merge queue Jul 11, 2023

Merged via the queue into dapr:main with commit 801847c Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(errors): add custom error classes #508

chore(errors): add custom error classes #508

jriffs commented Jun 19, 2023

shubham1172 Jun 19, 2023

jriffs Jun 19, 2023

shubham1172 Jun 20, 2023

jriffs Jun 20, 2023

shubham1172 Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

shubham1172 Jun 21, 2023

jriffs Jun 21, 2023

jriffs Jul 3, 2023

shubham1172 Jul 3, 2023

shubham1172 Jul 3, 2023

shubham1172 Jul 3, 2023

jriffs commented Jul 10, 2023

shubham1172 commented Jul 10, 2023

chore(errors): add custom error classes #508

chore(errors): add custom error classes #508

Conversation

jriffs commented Jun 19, 2023

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1172 Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jriffs commented Jul 10, 2023

shubham1172 commented Jul 10, 2023

shubham1172 Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading