CLDSRV-759: bucketGetRateLimit #5997
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 3 files with indirect coverage changes @@ Coverage Diff @@
## improvement/CLDSRV-766/bucket_rate_limiting #5997 +/- ##
===============================================================================
+ Coverage 83.43% 84.01% +0.57%
===============================================================================
Files 200 200
Lines 12508 12505 -3
===============================================================================
+ Hits 10436 10506 +70
+ Misses 2072 1999 -73
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| headers: request.headers, | ||
| }; | ||
|
|
||
| if (method !== 'GET' && method !== 'DELETE') { |
There was a problem hiding this comment.
It seems a bit odd to attach a body for all other types of requests (i.e. to have a negative check), I'd rather check if (method === 'PUT' || method === 'POST') instead.
| try { | ||
| const json = JSON.parse(text); | ||
| if (json.error) { | ||
| throw json; |
There was a problem hiding this comment.
This throw would be caught by the below catch, right? Not sure this is what you intended to do.
Also, do you want to throw an error in 100% of cases where !request.ok is true? In case it is so, the code doesn't throw in all cases (but maybe you did this on purpose so asking, in which case an extra comment after the try/catch block could help understanding this intent).
| it('should return NoSuchBucket error when bucket does not exist', async () => { | ||
| try { | ||
| await sendRateLimitRequest('GET', '127.0.0.1:8000', '/nonexistentbucket/?rate-limit'); | ||
| } catch (err) { |
There was a problem hiding this comment.
Maybe add an assert.fail here too like in the above test
No description provided.