[libstd]: add Dir.Iterator tests

[kubkon]

This commit adds some std.fs.Dir.Iterator tests.

Partially addresses #5653.

[kubkon]

SourceHut config seems broken -- see #5756.

[squeek502]

#5653 is proving its worth with every PR. 🙂 Looks like this test is uncovering another bug.

Windows is getting:

Entry{ .name = some_fil, .kind = Kind.Directory }
Entry{ .name = some_file, .kind = Kind.File }

EDIT: Within the loop, the entry is correct, it's only after it's appended to entries that it becomes corrupted. Looks like the name slice is being reused.

EDIT#2: Maybe not a bug, this is the doc comment for Dir.Iterator.next:

/// Memory such as file names referenced in this returned entry becomes invalid
/// with subsequent calls to `next`, as well as when this `Dir` is deinitialized.

EDIT#3: Alternate implementation that fixes the test on Windows:

test "Dir.Iterator" {
    var tmp_dir = tmpDir(.{ .iterate = true });
    defer tmp_dir.cleanup();

    // First, create a couple of entries to iterate over.
    const file = try tmp_dir.dir.createFile("some_file", .{});
    file.close();

    try tmp_dir.dir.makeDir("some_dir");

    var iter = tmp_dir.dir.iterate();
    var len: usize = 0;

    while (try iter.next()) |entry| {
        switch (entry.kind) {
            .File => testing.expectEqualSlices(u8, "some_file", entry.name),
            .Directory => testing.expectEqualSlices(u8, "some_dir", entry.name),
            else => unreachable,
        }
        len += 1;
    }

    testing.expect(len == 2); // note that the Iterator skips '.' and '..'
}

EDIT#4: Also worth noting that the doc comment for Dir.Iterator.next is missing in the Windows implementation, which should also be fixed.

[kubkon]

#5653 is proving its worth with every PR. 🙂 Looks like this test is uncovering another bug.

Windows is getting:

Entry{ .name = some_fil, .kind = Kind.Directory }
Entry{ .name = some_file, .kind = Kind.File }

EDIT: Within the loop, the entry is correct, it's only after it's appended to entries that it becomes corrupted. Looks like the name slice is being reused.

EDIT#2: Maybe not a bug, this is the doc comment for Dir.Iterator.next:

/// Memory such as file names referenced in this returned entry becomes invalid
/// with subsequent calls to `next`, as well as when this `Dir` is deinitialized.

EDIT#3: Alternate implementation that fixes the test on Windows:

test "Dir.Iterator" {
    var tmp_dir = tmpDir(.{ .iterate = true });
    defer tmp_dir.cleanup();

    // First, create a couple of entries to iterate over.
    const file = try tmp_dir.dir.createFile("some_file", .{});
    file.close();

    try tmp_dir.dir.makeDir("some_dir");

    var iter = tmp_dir.dir.iterate();
    var len: usize = 0;

    while (try iter.next()) |entry| {
        switch (entry.kind) {
            .File => testing.expectEqualSlices(u8, "some_file", entry.name),
            .Directory => testing.expectEqualSlices(u8, "some_dir", entry.name),
            else => unreachable,
        }
        len += 1;
    }

    testing.expect(len == 2); // note that the Iterator skips '.' and '..'
}

EDIT#4: Also worth noting that the doc comment for Dir.Iterator.next is missing in the Windows implementation, which should also be fixed.

Wow, thanks for the in-depth analysis @squeek502!

I've actually had the same observations just didn't get a chance to write that up yet. Indeed, name is reused per next call, however, this doesn't change the fact we have an inconsistency between hosts, so a doc comment specifically outlining this on Windows should be added.

Now, this still doesn't change the fact we should be able to make the test pass without resorting to testing for equality within the while loop. I reckon we should be able to copy out the bytes into an ArrayList (i.e., collect the entries) and continue on with the test as is, right? In fact, I assumed that ArrayList.append would do the copying for us since according to the docs:

/// Extend the list by 1 element. Allocates more memory as necessary.

It's obviously not the case. Anyhow, I tried copying the bytes out manually yesterday, and I'd still get memory corruption somewhere for some reason. It was late though, so maybe I'll have better luck at some point later today. If anything comes back, I shall drop a line or two here.

EDIT: OK, ArrayList can't do the item allocation and copying for us since we don't necessarily deal with packed structs as is the case with Dir.Entry. Essentially, as far as I understand this, we have to do the deepcopy ourselves and then push the result into the ArrayList which should solve the problem of sharing the pointer to the name buffer in the Dir.Iterator.

[kubkon]

OK, 9b29218 should fix the memory corruption bug in the test. While we're here, do you think it would be useful to have Iterator.nextAlloc(self: *Self, allocator: *Allocator) method which internally would call next(), and allocate and copy the entry into the newly allocated space?

EDIT: OK, I've now submitted an issue to track this proposal: #5768

[kubkon]

Unless anyone else has anything else to add, could we merge this into upstream?

[squeek502]

Indeed, name is reused per next call, however, this doesn't change the fact we have an inconsistency between hosts, so a doc comment specifically outlining this on Windows should be added.

Might be worth addressing this in this PR.

[kubkon]

Indeed, name is reused per next call, however, this doesn't change the fact we have an inconsistency between hosts, so a doc comment specifically outlining this on Windows should be added.

Might be worth addressing this in this PR.

Good catch, thanks! Done in 417c928.