Checksum tests #9
Conversation
|
@avinassh, Can you pls check this small PR once? Tests for the checksum operation! |
| @@ -184,7 +183,7 @@ func (d *DiskStore) Delete(key string) error { | |||
|
|
|||
| // mark as tombstone | |||
| h.MarkTombStone() | |||
| r := Record{Header: h, Key: key, Value: "", RecordSize: headerSize + h.KeySize + h.ValueSize} | |||
| r := Record{Header: h, Key: key, Value: value, RecordSize: headerSize + h.KeySize + h.ValueSize} | |||
There was a problem hiding this comment.
hey, did this cause a regression? Why did we change
There was a problem hiding this comment.
No any regression. Just used the var defined on line 181:
value := ""
disk_store_test.go
Outdated
| t.Errorf("invalid checksum: Got: %d, Want: %d", actualCheckSum, expectedCheckSum) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
could you tell me the intention behind this test
There was a problem hiding this comment.
I added explicit tests to check for valid and invalid checksums both. Is it overkill?
There was a problem hiding this comment.
yup, because I feel these are not doing anything. Can you think of a case where this test can help catch a bug?
disk_store_test.go
Outdated
|
|
||
| // corrupt the record by overriding few bytes with corruptedBytes | ||
| corruptedBytes := []byte{12, 90, 87, 101} | ||
| start, end := 17, 21 |
There was a problem hiding this comment.
I think you can avoid hard coding these and consider last 4 bytes
| r3 := Record{Header: h3, Key: k3, Value: v3, RecordSize: headerSize + h3.KeySize + h3.ValueSize} | ||
| r3.Header.CheckSum = r3.CalculateCheckSum() | ||
|
|
||
| tests := []Record{r1, r2, r3} |
There was a problem hiding this comment.
why 3 records? it seems all three iterations doing same thing and testing for same
There was a problem hiding this comment.
Considered 3 different records to test for different lengths of k,v pairs. Should I change to use only one k,v pair?
There was a problem hiding this comment.
no it is fine, I'd rather keep it 😅
No description provided.