Wiredcraft / test-backend

39 stars 76 forks source link

APIs for get/create/update/delete user data #93

Closed dd201702 closed 2 years ago

dd201702 commented 2 years ago

Please refer to docs/how-to-run.md for how I designed and how to run the project.

MiffyLiye commented 2 years ago

Hello, I'm Tao from Wiredcraft, I'm going to review the PR and ask some questions.

  1. In test cases, finding user with non existed id got 200 status code, which is not typical RESTful API style. Can you compare the pros and cons of typical RESTful status code, and always 200 status code?
  2. The test cases depends on test data prepared in previous test cases, making the test cases not independent. The F.I.R.S.T. principles suggest test cases to be Isolated/Independent. Can you compare benefit and cost of writing dependent and independent tests? Are there ways to reduce the cost in each case?
dd201702 commented 2 years ago

Hello, I'm Tao from Wiredcraft, I'm going to review the PR and ask some questions.

1. In test cases, finding user with non existed id got 200 status code, which is not typical RESTful API style. Can you compare the pros and cons of typical RESTful status code, and always 200 status code?

2. The test cases depends on test data prepared in previous test cases, making the test cases not independent. The F.I.R.S.T. principles suggest test cases to be `Isolated/Independent`. Can you compare benefit and cost of writing dependent and independent tests? Are there ways to reduce the cost in each case?

Hello Tao, thank you for give me feedback. For question 1, I agree with you that non existed id should not get 200 status code. So I update function code to return response as below format. I did similar changes for update by id. And update related unit tests.

{
  "statusCode": 404,
  "message": "Not Found"
}

For question 2, my unit tests are not Isolated/Independent, should use beforeEach and afterEach to prepare and clean db data for each unit test. My current test cases are easily to verify functions can be step by step in one time, but bad if some one case failed the rest cases will be affected. But sorry I don't have time to fix my code today. If possible I will try to fix it in several days.

MiffyLiye commented 2 years ago

Hello Tao, thank you for give me feedback. For question 1, I agree with you that non existed id should not get 200 status code. So I update function code to return response as below format. I did similar changes for update by id. And update related unit tests.

{
  "statusCode": 404,
  "message": "Not Found"
}

For question 2, my unit tests are not Isolated/Independent, should use beforeEach and afterEach to prepare and clean db data for each unit test. My current test cases are easily to verify functions can be step by step in one time, but bad if some one case failed the rest cases will be affected. But sorry I don't have time to fix my code today. If possible I will try to fix it in several days.

you can also document the new strategy in comments, not necessary to implement.

dd201702 commented 2 years ago

you can also document the new strategy in comments, not necessary to implement.

Got you. To make my unit tests be Isolated/Independent, could use current mongo db or mock mongo db, then use before/beforeEach to prepare test data and after/afterEach to clean test data.

dd201702 commented 2 years ago

Hello Tao, thank you for give me feedback. For question 1, I agree with you that non existed id should not get 200 status code. So I update function code to return response as below format. I did similar changes for update by id. And update related unit tests.

{
  "statusCode": 404,
  "message": "Not Found"
}

For question 2, my unit tests are not Isolated/Independent, should use beforeEach and afterEach to prepare and clean db data for each unit test. My current test cases are easily to verify functions can be step by step in one time, but bad if some one case failed the rest cases will be affected. But sorry I don't have time to fix my code today. If possible I will try to fix it in several days.

you can also document the new strategy in comments, not necessary to implement.

I just add several unit tests, using before and after to make them Isolated/Independent.

xavierchow commented 2 years ago

Thanks all, this PR is good enough for our evaluation.

@dd201702 we will get back to you soon.