Test non-empty StorageIDs #8698
Conversation
itaigilo
added
the
exclude-changelog
itaigilo
requested review from
N-o-Z,
arielshaqed,
nadavsteindler and
guy-har
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
There was a problem hiding this comment.
THANKS! Much needed unit tests. But we might need more; these might be in a future PR, but then this PR has to say that and we need a more specific issue.
- Not sure how this tests CreateRepository.
- Don't we need to update other tests? E.g. graveler_v2_test.go appears to pass empty storage IDs; there may be others.
Blocking on these 3 issues (including the one on catalog_test.go). Entirely possible I misunderstood, and will happily unblock.
| @@ -35,16 +36,16 @@ func TestManager_WriteRange(t *testing.T) { | |||
| }{ | |||
| { | |||
| name: "iterator_exhausted", | |||
| initStorageID: "", | |||
| writeStorageID: "", | |||
| initStorageID: config.SingleBlockstoreID, | |||
There was a problem hiding this comment.
I don't understand what we did in this test, but also what we did in the other test files. I would have expected to have twice as many test cases: once for "" aka SIngleBlockstoreID, one for "sid1". (This could have happened with nested test.Run()s, of course.) But instead some test cases switch to testing "", others to "sid1". And I don't understand why.
Obviously I don't understand this for all changes in this file, not only for this one.
There was a problem hiding this comment.
As far as I can understand, we're trying to make sure StorageID flows properly from start to finish. By starting with either "" or "sid*" on one side, and making sure we're getting the same result in the other end, we have some confidence it's being passed properly, no?
| @@ -37,3 +39,9 @@ func InitGravelerTest(t *testing.T) *GravelerTest { | |||
|
|
|||
| return test | |||
| } | |||
|
|
|||
| // ShortenBranchUpdateBackOff upgrade graveler branch update back-off to shorten test duration | |||
There was a problem hiding this comment.
Found out the graveler tests were way too slow -
Ended up adding this function to this PR.
Probably belongs to a separate one, but I think we can handle this.
(Anyway, this cuts running time from 90 seconds to only 8.)
Thanks @arielshaqed fro your review!
|
Closes #8697.
Change Description
In order to provide better coverage:
config.SingleBlockstoreID.