search

Wiredcraft / test-backend

39 stars 76 forks source link

[冯春博] feat(server): 用户后端及接口文档 #94

Closed FengBryan closed 2 years ago

MiffyLiye commented 2 years ago

Hello, I'm Tao from Wiredcraft, I will review the code and ask some questions.

  1. The API document and test cases didn't mention error cases, especially client side error cases. In typical REST API, what should be the response for error cases, can you give some examples?
  2. In test cases, the register API uses userName field name, while login API uses username field name, while in API document, they both use userName. Can you suggest how to know if API document is up to date?
  3. How do you arrange the test case's Given-When-Then in the test code? What type of information should be asserted in Then part?
  4. In test cases, register API and login API seems to reuse same userId, but doesn't use same password, and login can also get 201 status code. Can you find the root cause why password verification is skipped?
FengBryan commented 2 years ago

@MiffyLiye I will patch a commit for issues

MiffyLiye commented 2 years ago

@MiffyLiye I will patch a commit for issues

it's OK to propose a plan or analyze the root cause in comments, not necessary to implement.

FengBryan commented 2 years ago

@MiffyLiye PTAL

MiffyLiye commented 2 years ago

@FengBryan The implementation looks OK. Can you choose one open question below and provide some thoughts? Just share some thoughts, doesn't need to be perfect.

FengBryan commented 2 years ago

@FengBryan The implementation looks OK. Can you choose one open question below and provide some thoughts? Just share some thoughts, doesn't need to be perfect.

  • Can you suggest how to know if API document is up to date?
  • Can you find the root cause why password verification is skipped? How to help other team members avoid this in the future?

Can you suggest how to know if API document is up to date?

  1. With code change, decorator will update metadata to its controller class, and api server will get newest metadata when use Reflect, so the swagger doc will be up to date. This is automatic, no need to change manually
  2. If there will be a new release and will not compatible for previous api define, will should reword api path with new version such as v1 => v2. If previous version will be deprecated, should note the doc item to deprecate

Can you find the root cause why password verification is skipped? How to help other team members avoid this in the future?

  1. This is a low-level mistake i made, because i use bcrypt module for password validate, but i forgot it behavior. I thought it will throw error when compared fail. But it returned a boolean to sign success or fail. I just use compare but missed the return value, so it will login successfully when given invalid user info.
  2. To avoid this problem, I think the best way is more and more test cases to cover all branches/scenarios.
xavierchow commented 2 years ago

Thanks guys, very thorough discussion here, pretty good enough for our evaluation. I'm closing this PR, we will get back to you soon @FengBryan