-
-
Notifications
You must be signed in to change notification settings - Fork 3k
std.fs.File: fix positional/streaming seekBy #25095
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
base: master
Are you sure you want to change the base?
Conversation
8c44618
to
8e85bf2
Compare
8e85bf2
to
10ddf27
Compare
10ddf27
to
d967f94
Compare
28824c4
to
109199d
Compare
109199d
to
1d5637d
Compare
77a916a
to
401c6f5
Compare
401c6f5
to
f75fd96
Compare
311477a
to
47b1c31
Compare
The main thing I would say reading through the PR, is that the tests seem to be testing the internal behaviour of the reader implementation (how the buffering works etc.) depending on modes and I don't think that's super important. Ideally they would be testing that actual API boundary/contract. Which is that when seeking, the logical pos in the file changes as expected and that the data read through calls to the reader API return the data that is expected. So instead of tests like: // seek within bufferedLen
const pos_within = reader.pos;
try reader.seekBy(2);
// wasi does not buffer in this case
if (builtin.target.os.tag != .wasi) {
try testing.expectEqual(pos_within, reader.pos);
} I personally would test for: const logical_pos_before = reader.logicalPos();
try reader.seekBy(2);
try testing.expectEqual(logical_pos_before + 2, reader.logicalPos()); (and then that the buffer returns the correct data, but that is already being done) Maybe testing that these all work based on seeking within and beyond the buffer could still be useful as they invoke different internal logic. But I don't think checking that we get specific values into the inteferace Also adding a negative seekBy to the test could be useful as well for: #25020 This is all just my opinion though so take it with a grain of salt (as others might have different thoughts)! And I hope it makes sense as I went on a couple tangents. |
b3ceec7
to
9700149
Compare
Updated tests to not test internals, just the interface. I also added a negative seekBy. This was the version of the tests that made sure, that the buffer is discarded. I am now pretty sure that testing internals was a bad idea. |
9700149
to
2d6bd3e
Compare
positional seekBy should use the logical position, but it used the file position which includes the buffered data. the test did not catch this because it used an empty buffer. fixes ziglang#25087 ziglang#25020
streaming seekBy completely ignored the buffer. the new logic checks if the seek offset is smaller than the buffer and adds the offset to r.interface.seek. otherwise the old logic is invoked.
2d6bd3e
to
48cfe69
Compare
commit 1: seekBy should use the logical position, but it used the file position which includes the buffered data. the test did not catch this because it used an empty buffer. fixes #25087 #25020
commit 2: streaming seekBy completely ignored the buffer. the new logic checks if the seek offset is smaller than the buffer and adds the offset to r.interface.seek. otherwise the old logic is invoked.