-
Notifications
You must be signed in to change notification settings - Fork 80
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
QA: test - user registration backend api #59
QA: test - user registration backend api #59
Conversation
@anitab-org/bridgeintech-maintainers , can you please advice if this is ok? only done one test for now coz I want to see if it's ok before writing the next test cases. Also, there's issue with travis build as i mentioned above. can you pls guide me on how to troubleshoot this? |
I just realized that the response from With latest code (masked response) So, I'm just fixing the actual code at the same time (thanks to testing, we catch the bug early) 😂. Although just now I changed the code back to matching old code for I just didn't have time to write a test for this random exception since my focus is to test the listed items first for the time constraint sake. cc @anitab-org/bridgeintech-maintainers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked the tests covered(good coverage) and the Travis build is also passing.
@ramitsawhney27 can you review? |
Thanks, @meenakshi-dhanani for your review. 😉 |
Update @anitab-org/bridgeintech-maintainers. I've just added validation check to Register user functionality. forgot to put it there for some reason 😊 |
app/api/resources/users.py
Outdated
|
||
if is_valid != {}: | ||
return is_valid, HTTPStatus.BAD_REQUEST | ||
|
||
# send POST /register request to MS API and return response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line gaps seem inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, fixed it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update @anitab-org/bridgeintech-maintainers. I've modified all the requested changes. Can you please re-review? Thanks |
fc07e9d
to
966fd7f
Compare
966fd7f
to
1f3e8a9
Compare
1f3e8a9
to
9c0621a
Compare
@meenakshi-dhanani , I've just push the latest change as per your request. Can you please re-review? Thanks |
Why did we need to force push? Not sure. It could be a separate commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis build passes. Tests coverage looks fine to me and code has been reviewed by Ramit.
@ramitsawhney27 can you re-review? There was just a formatting change I had asked for. I think that's why you might need to review again |
Like I mentioned in our call last night, since I opened new branch from develop to work on namedtuple, I need to pull rebase from this PR #59 branch and PR #60 to make sure my changes for the name tuple won't break the register functionality and its tests. If both PR I pull (59 and 60) have separate commits, those commits from 2 separate branches will intermingle based on whichever got committed first, and it won't be clear where individual commit coming from. That's why I squashed commits into one on each PR and pull rebase it from my namedtuple branch. You can see from the screenshot below: Can you see why it's better to squash commits in this case? As you can see, it is clear where the commits are coming from (PR #59 Register, #56 from develop branch #60 Test Register). If I left commits separately from PR59 and PR60 branches, you'll see lots of small commits scattered everywhere and not sure where they're coming from (I knew this coz that's the issue I used to face when I started a new PR on top of code base of another PR that is still being reviewed). since I squash merge, this intertwined commits no longer an issue. |
@foongminwong . Try to set the search path on your bit_schema_test database (actually set search path for both bit_schema and bit_schema_test) from your command line, just like what Meena and I did in the travis script below |
Problem with small commits scattered everywhere - Let's understand why small commits are important. They help your code become easier to understand, basically dividing your work into small parts. The problem with these being scattered is because they might belong to different PRs, in that case what we usually do is establish a commit format, eg. [PR #] so you can always look at a commit and know what PR it is referring to. About why the small commits might not look that good, is because the messages are not meaningful. If a message is meaningful and indicative of intent, it's always useful to look at those small commits. In teams and projects, since multiple things are being developed in parallel, you will have occasions when there's a sequence such as this: #A There is no need to squash all A's together. And force push and change this. The reality is that work is being done in a parallel fashion, let your history show that. Conclusion: Have clean commit messages, stick to a format. At work we often use |
Ok. I'll do it moving forward then. Thanks for the advice, @meenakshi-dhanani 😉 |
9c0621a
to
85c82fb
Compare
Update @anitab-org/bridgeintech-maintainers. I have pull rebased the recent merged to develop branch on this PR. |
Description
test cases for Register user API
Fixes #54
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
python -m unittest discover tests
on the terminalBoth resulted in pass
Note: To test the register UI-backend integration/functionality manually, you MUST run BIT frontend server with the PR#24 latest code otherwise it will not work.
Checklist:
Code/Quality Assurance Only
Additional Note:
Travis-build failed but caused by unrelated issue (can't create test database through Travis)