server: update TestRangeResponse unit test to work correctly with secondary tenant#139841
Conversation
|
This change is |
69bfe7e to
cbead90
Compare
cbead90 to
671c066
Compare
shubhamdhama
left a comment
There was a problem hiding this comment.
Congrats on your first PR 🎉
It's a great start. I have left some minor comments.
| t.Run("access range endpoint within current tenant's scope", func(t *testing.T) { | ||
| // Test case 1, looking for a range id which lies within the current tenant's (system or secondary tenant, chosen probabilistically) range | ||
| var currentTenantRangeId int64 // could be a system tenant or a secondary application tenant | ||
| t.Logf("Is system tenant enabled : %v", isSystemTenant) |
There was a problem hiding this comment.
I think we can remove all these log statements (I'm not very convinced if they are adding value here).
|
I have some suggestions for the commit message too,
|
|
nit: fix the commit title as per https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages#Commit-title. |
Done
I wanted to link the pr with the jira ticket, hence used |
done 👍 |
671c066 to
d45fe1f
Compare
cthumuluru-crdb
left a comment
There was a problem hiding this comment.
Congrats on your first PR!
There was a problem hiding this comment.
Consider require.ErrorContains() with "wrong number of ranges in the response" substring.
There was a problem hiding this comment.
This is a custom message which is added as part of the unit test returned from the invocation of validateRangeBelongsToTenant(). Sticking to full error message comparison as this error message is something which is articulated as part of this test and not something returned from the applicationServer or any other such component.
There was a problem hiding this comment.
Yeah, I saw that. Please consider that comment as a nit. You can also checkout require.EqualError() and require.EqualErrorf() if any of them fit better here.
shubhamdhama
left a comment
There was a problem hiding this comment.
LGTM!
Left some nits, otherwise looks good.
75cde0a to
bcd8e72
Compare
|
I think you need to fixup last two commits into the first commit. The end state should contain only one commit. |
…ctly with secondary application tenant
What was there before: the change focuses on updating the unit test which was failing with secondary tenant while hitting the
_status/range/{id} endpoint. This was happening because the the test had a hard-coded range id `1` to be
fetched from the aforementioned endpoint. And this range id lies outside the scope of a secondary application
tenant.
resolution: made changes to the test to include a range id which lies well within the scope of the secondary tenant.
Fixes: cockroachdb#110018
Part of: https://cockroachlabs.atlassian.net/browse/CRDB-31224
Epic: CRDB-38968
Release note: None
bcd8e72 to
e20f0a5
Compare
|
bors r=shubhamdhama,cthumuluru-crdb |
|
Thank you @shubhamdhama @cthumuluru-crdb for your feedback and reviews on my first pr. |
server/storage_api: update TestRangeResponse unit test, to work correctly with secondary application tenant
What was there before: the change focuses on updating the unit test which was failing with secondary tenant while hitting the
_status/range/{id} endpoint. This was happening because the the test had a hard-coded range id
1to befetched from the aforementioned endpoint. And this range id lies outside the scope of a secondary application
tenant.
resolution: made changes to the test to include a range id which lies well within the scope of the secondary tenant.
Fixes: #110018
Part of: https://cockroachlabs.atlassian.net/browse/CRDB-31224
Epic: CRDB-38968
Release note: None