Improvement/s3 utils 191 #337
Conversation
benzekrimaha
force-pushed
the
improvement/S3UTILS-191
branch
3 times, most recently
from
900271a to
02d89bc
Compare
| @@ -28,7 +28,7 @@ | |||
| "dependencies": { | |||
| "@senx/warp10": "^2.0.3", | |||
| "JSONStream": "^1.3.5", | |||
| "arsenal": "git+https://github.com/scality/arsenal#8.2.8", | |||
| "arsenal": "git+https://github.com/scality/arsenal#22a5462d7d9bdec0d21a0e9fb484594af35c1cee", | |||
There was a problem hiding this comment.
Please note that this will be switched to the arsenal adequate version once the current in review PR with a fix is merged: scality/Arsenal#2371
benzekrimaha
force-pushed
the
improvement/S3UTILS-171-bump-deps
branch
from
254ba0b to
ba16cc2
Compare
benzekrimaha
force-pushed
the
improvement/S3UTILS-191
branch
from
02d89bc to
3d693cb
Compare
benzekrimaha
force-pushed
the
improvement/S3UTILS-191
branch
from
4fbf388 to
6ca91b6
Compare
This commit actually implements the _getIsTransient and _pensieveLocationIsTransient methods in the CountWorker.js previously implemented on arsenal and only used by the CountWorker.js. Issue: S3UTILS-191
benzekrimaha
force-pushed
the
improvement/S3UTILS-191
branch
from
afb8abd to
1c533be
Compare
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| @@ -180,13 +174,11 @@ describe('CountItems::CountWorker', () => { | |||
| sendFn: testSendFn, | |||
| client: mongoMock, | |||
| }); | |||
| w.getIsTransient = jest.fn((bucketInfo, cb) => cb(null, true)); | |||
There was a problem hiding this comment.
just mocking getIsTransient means we basically do not execute this code...
- Best to have some test exercising the "whole" code
- If needed, we may use a mock (in some tests, ideally not all) to simulate specific conditions/results/...
- In any case, need extra tests cases to verify the new functions behave somewhat correctly (esp. do not crash) and the information ("is transient") is used appropriately
There was a problem hiding this comment.
There was a problem hiding this comment.
Looking at the tests, it does not cover all branches of the code.
In addition, these tests are overly complicated to tests 2 very simple functions : would seem much easier (both to write and to review) to have "simpler" tests to exercise all code path in getIsTransient() (and the nested pensieveLocationIsTransient) by just calling this function.
| mongoMock.getObjectMDStats.mockImplementationOnce((_a, _b, _c, _d, cb) => cb(null, { value: 42 })); | ||
| w.countItems(bucketInfo, (err, results) => { | ||
| expect(err).toBeNull(); | ||
| expect(results).toEqual({ value: 42 }); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| test('should use real getIsTransient and not crash', done => { |
There was a problem hiding this comment.
should have at least 2 scenarios, depending on this.client.isLocationTransient (since there are multiple code path).
the point is not just to see that countItems is calling getIsTransient, but really to execute the code in getIsTransient to ensure it will not fail in production... (it may be done though other tests, calling the function directly!)
(alternatively, since we use a wrapper of MongoClient in here: if you add the function there they can be tested in mongoclient tests, and you don't need to change the countWorker tests)
benzekrimaha
force-pushed
the
improvement/S3UTILS-191
branch
from
796b65a to
f8c0988
Compare
| @@ -180,13 +174,11 @@ describe('CountItems::CountWorker', () => { | |||
| sendFn: testSendFn, | |||
| client: mongoMock, | |||
| }); | |||
| w.getIsTransient = jest.fn((bucketInfo, cb) => cb(null, true)); | |||
There was a problem hiding this comment.
Looking at the tests, it does not cover all branches of the code.
In addition, these tests are overly complicated to tests 2 very simple functions : would seem much easier (both to write and to review) to have "simpler" tests to exercise all code path in getIsTransient() (and the nested pensieveLocationIsTransient) by just calling this function.
| test('should use real getIsTransient and not crash', done => { | ||
| const testSendFn = jest.fn(); |
There was a problem hiding this comment.
test name feels weird : there is no "feature" of the code to use a "real getIsTransient", it is really just that getIsTransient is mocked in all tests...
--> should find a name that better matches what "feature" of the code this test exercises (or take this as a hint that indeed we are mixing approaches here, and we shoudl either not use a mock in the tests -focus on e2e function, covering all cases in data-, or keep the mock to test the "counting" part and create other tests to verify getIsTransient works as expected)
| @@ -22,7 +21,6 @@ describe('CountItems::CountWorker', () => { | |||
| setup: [null], | |||
| close: [], | |||
| isConnected: false, | |||
| getIsTransient: [], | |||
There was a problem hiding this comment.
in the previous code, we used to have different getIsTransient() mock results depending on the test case : i.e. either [] (i.e. no error, not transient) or [null, true] (i.e. no error, transient)
However, with your change the mock always returns true : and we don't actually test the "usual" scenario we are in...
Issue: S3UTILS-191