-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Add BytesStreamOutputTests #134788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add BytesStreamOutputTests #134788
Conversation
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple suggestions, but looks good 👍
server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamOutputTests.java
Show resolved
Hide resolved
byte[] byteArray = randomByteArrayOfLength(byteArrayLength); | ||
stream.writeBytes(byteArray, 0, byteArrayLength); | ||
|
||
int seekPosition = randomIntBetween(byteArrayLength, byteArrayLength + 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, a seek could be done to somewhere between 0 and byteArrayLength
, and then a read can be done to show that it matches the byteArray
? Same thing with skip below, though the seek would have to be further constrained to leave some room to test skip on the data set.
Would that be more realistic than setting the read position into space that hasn't been written yet? Or maybe test both?
While writing #133558 I adopted an approach that utilised
BytesStreamOutput
and I wrote a unit test suite. Subsequent refactors have removed the need to use this class, but I still believe there is value in having theBytesStreamOutputTests
suite.Testing
I have ran this test suite through a 10000 times, and it all succeeds