#734 : Http Commands Error Messages Test #900
Conversation
svkaizoku
changed the title
|
@lucifercr07 @pratikpandey21 , feel free to review this whenever free. Thanks 😄 |
svkaizoku
changed the title
|
Hi @svkaizoku, thanks for the changes. I have reviewed the HTTP test file. However, I suggest reverting the changes due to code indentation. Those changes deviate from the primary objective of the PR and its issue. We can take that up as a separate issue or PR if needed. This is just a suggestion. |
Hey @apoorvyadav1111 , thanks for the suggestion. It makes perfectly sense to not combine this in this PR. I was just following the contributing guidelines https://github.com/DiceDB/dice/blob/master/CONTRIBUTING.md#code-formatting. Maybe we should just format the files that we change. Maybe we should add that explicitly to the documentation. I have reverted the changes btw. Feel free to take a look at it again. Thanks :) |
| time.Sleep(tc.delays[i]) | ||
| } | ||
| result, _ := exec.FireCommand(cmd) | ||
| if len(tc.assertType) == 0 { |
There was a problem hiding this comment.
Thanks for taking up my suggestion. Yes, the code format for the file was not as per the requirements, but there are lot of other PRs with the same change, I am sure one of those will have it fixed before this is merged. I have question with thisif condition, given that none of the test cases have an empty array.
Thanks
There was a problem hiding this comment.
Thanks for the clarification. Let me remove that if here also. Thanks for pointing out.
There was a problem hiding this comment.
I will still be keeping the switch case for the asserts for furture use cases.
Fixes: #734
I will be splitting the implementation of this issue in multiple PR's. This PR contains tests similar to
check_type_test.goNote: There are changes to extra files due to linting