From c30bffbd0fe8b32295f48eda5a8b0972dd40c666 Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Sun, 29 Oct 2023 12:01:26 +0000 Subject: [PATCH] Add additional tests for filesystems/mounts This tries to cover some holes in our existing coverage. - Port some of our Java readable handle tests to Lua (and also clean up the Java versions to stop using ObjectWrapper - that dates to pre-@LuaFunction!) - Test a couple of discrepancies between binary and text handles. This is mostly to do with the original number-based .read() and .write() interface for binary handles. - Fix a couple of edge cases in file-size accounting. --- .../core/filesystem/WritableFileMount.java | 51 +++----- .../core/apis/ObjectWrapper.java | 49 ------- .../handles/BinaryReadableHandleTest.java | 52 ++++---- .../handles/EncodedReadableHandleTest.java | 36 +++--- .../filesystem/WritableFileMountTest.java | 10 ++ .../resources/test-rom/spec/apis/fs_spec.lua | 122 +++++++++++++++++- .../filesystem/WritableMountContract.java | 24 ++++ 7 files changed, 220 insertions(+), 124 deletions(-) delete mode 100644 projects/core/src/test/java/dan200/computercraft/core/apis/ObjectWrapper.java diff --git a/projects/core/src/main/java/dan200/computercraft/core/filesystem/WritableFileMount.java b/projects/core/src/main/java/dan200/computercraft/core/filesystem/WritableFileMount.java index b3626acef..7ebfe5c69 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/filesystem/WritableFileMount.java +++ b/projects/core/src/main/java/dan200/computercraft/core/filesystem/WritableFileMount.java @@ -175,7 +175,7 @@ public class WritableFileMount extends FileMount implements WritableMount { usedSpace += MINIMUM_FILE_SIZE; try { - return new CountingChannel(Files.newByteChannel(file, WRITE_OPTIONS), MINIMUM_FILE_SIZE, true); + return new CountingChannel(Files.newByteChannel(file, WRITE_OPTIONS), true); } catch (IOException e) { throw remapException(path, e); } @@ -195,11 +195,7 @@ public class WritableFileMount extends FileMount implements WritableMount { // Allowing seeking when appending is not recommended, so we use a separate channel. try { - return new CountingChannel( - Files.newByteChannel(file, APPEND_OPTIONS), - Math.max(MINIMUM_FILE_SIZE - (attributes == null ? 0 : attributes.size()), 0), - false - ); + return new CountingChannel(Files.newByteChannel(file, APPEND_OPTIONS), false); } catch (IOException e) { throw remapException(path, e); } @@ -207,31 +203,33 @@ public class WritableFileMount extends FileMount implements WritableMount { private class CountingChannel implements SeekableByteChannel { private final SeekableByteChannel channel; - private long ignoredBytesLeft; private final boolean canSeek; - CountingChannel(SeekableByteChannel channel, long bytesToIgnore, boolean canSeek) { + CountingChannel(SeekableByteChannel channel, boolean canSeek) { this.channel = channel; - ignoredBytesLeft = bytesToIgnore; this.canSeek = canSeek; } @Override public int write(ByteBuffer b) throws IOException { - count(b.remaining()); - return channel.write(b); - } + var toWrite = b.remaining(); - void count(long n) throws IOException { - ignoredBytesLeft -= n; - if (ignoredBytesLeft < 0) { - var newBytes = -ignoredBytesLeft; - ignoredBytesLeft = 0; - - var bytesLeft = capacity - usedSpace; - if (newBytes > bytesLeft) throw new IOException(OUT_OF_SPACE); - usedSpace += newBytes; + // If growing the file, make sure we have space for it. + var newPosition = Math.addExact(channel.position(), toWrite); + var newBytes = newPosition - Math.max(MINIMUM_FILE_SIZE, channel.size()); + if (newBytes > 0) { + var newUsedSpace = Math.addExact(usedSpace, newBytes); + if (newUsedSpace > capacity) throw new IOException(OUT_OF_SPACE); + usedSpace = newUsedSpace; } + + var written = channel.write(b); + + // Some safety checks to check our file size accounting is reasonable. + if (written != toWrite) throw new IllegalStateException("Not all bytes were written"); + assert channel.position() == newPosition : "Position is consistent"; + + return written; } @Override @@ -248,16 +246,7 @@ public class WritableFileMount extends FileMount implements WritableMount { public SeekableByteChannel position(long newPosition) throws IOException { if (!isOpen()) throw new ClosedChannelException(); if (!canSeek) throw new UnsupportedOperationException("File does not support seeking"); - if (newPosition < 0) { - throw new IllegalArgumentException("Cannot seek before the beginning of the stream"); - } - - var delta = newPosition - channel.position(); - if (delta < 0) { - ignoredBytesLeft -= delta; - } else { - count(delta); - } + if (newPosition < 0) throw new IllegalArgumentException("Cannot seek before the beginning of the stream"); return channel.position(newPosition); } diff --git a/projects/core/src/test/java/dan200/computercraft/core/apis/ObjectWrapper.java b/projects/core/src/test/java/dan200/computercraft/core/apis/ObjectWrapper.java deleted file mode 100644 index 7c8ebedde..000000000 --- a/projects/core/src/test/java/dan200/computercraft/core/apis/ObjectWrapper.java +++ /dev/null @@ -1,49 +0,0 @@ -// SPDX-FileCopyrightText: 2018 The CC: Tweaked Developers -// -// SPDX-License-Identifier: MPL-2.0 - -package dan200.computercraft.core.apis; - -import dan200.computercraft.api.lua.ILuaContext; -import dan200.computercraft.api.lua.LuaException; -import dan200.computercraft.api.lua.LuaTask; -import dan200.computercraft.api.lua.ObjectArguments; -import dan200.computercraft.core.asm.LuaMethodSupplier; -import dan200.computercraft.core.methods.LuaMethod; -import dan200.computercraft.core.methods.MethodSupplier; - -import java.util.List; -import java.util.Map; - -public class ObjectWrapper implements ILuaContext { - private static final MethodSupplier LUA_METHODS = LuaMethodSupplier.create(List.of()); - - private final Object object; - private final Map methodMap; - - public ObjectWrapper(Object object) { - this.object = object; - methodMap = LUA_METHODS.getSelfMethods(object); - } - - public Object[] call(String name, Object... args) throws LuaException { - var method = methodMap.get(name); - if (method == null) throw new IllegalStateException("No such method '" + name + "'"); - - return method.apply(object, this, new ObjectArguments(args)).getResult(); - } - - @SuppressWarnings({ "unchecked", "TypeParameterUnusedInFormals" }) - public T callOf(String name, Object... args) throws LuaException { - return (T) call(name, args)[0]; - } - - public T callOf(Class klass, String name, Object... args) throws LuaException { - return klass.cast(call(name, args)[0]); - } - - @Override - public long issueMainThreadTask(LuaTask task) { - throw new IllegalStateException("Method should never queue events"); - } -} diff --git a/projects/core/src/test/java/dan200/computercraft/core/apis/handles/BinaryReadableHandleTest.java b/projects/core/src/test/java/dan200/computercraft/core/apis/handles/BinaryReadableHandleTest.java index 42cb04b8d..2e8a2ce79 100644 --- a/projects/core/src/test/java/dan200/computercraft/core/apis/handles/BinaryReadableHandleTest.java +++ b/projects/core/src/test/java/dan200/computercraft/core/apis/handles/BinaryReadableHandleTest.java @@ -5,71 +5,77 @@ package dan200.computercraft.core.apis.handles; import dan200.computercraft.api.lua.LuaException; -import dan200.computercraft.core.apis.ObjectWrapper; import org.junit.jupiter.api.Test; +import javax.annotation.Nullable; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.Optional; import static org.junit.jupiter.api.Assertions.*; public class BinaryReadableHandleTest { @Test public void testReadChar() throws LuaException { - var wrapper = fromLength(5); - assertEquals('A', (int) wrapper.callOf(Integer.class, "read")); + var handle = fromLength(5); + assertEquals('A', cast(Integer.class, handle.read(Optional.empty()))); } @Test public void testReadShortComplete() throws LuaException { - var wrapper = fromLength(10); - assertEquals(5, wrapper.callOf("read", 5).remaining()); + var handle = fromLength(10); + assertEquals(5, cast(ByteBuffer.class, handle.read(Optional.of(5))).remaining()); } @Test public void testReadShortPartial() throws LuaException { - var wrapper = fromLength(5); - assertEquals(5, wrapper.callOf("read", 10).remaining()); + var handle = fromLength(5); + assertEquals(5, cast(ByteBuffer.class, handle.read(Optional.of(10))).remaining()); } @Test public void testReadLongComplete() throws LuaException { - var wrapper = fromLength(10000); - assertEquals(9000, wrapper.callOf("read", 9000).length); + var handle = fromLength(10000); + assertEquals(9000, cast(byte[].class, handle.read(Optional.of(9000))).length); } @Test public void testReadLongPartial() throws LuaException { - var wrapper = fromLength(10000); - assertEquals(10000, wrapper.callOf("read", 11000).length); + var handle = fromLength(10000); + assertEquals(10000, cast(byte[].class, handle.read(Optional.of(11000))).length); } @Test public void testReadLongPartialSmaller() throws LuaException { - var wrapper = fromLength(1000); - assertEquals(1000, wrapper.callOf("read", 11000).remaining()); + var handle = fromLength(1000); + assertEquals(1000, cast(ByteBuffer.class, handle.read(Optional.of(11000))).remaining()); } @Test public void testReadLine() throws LuaException { - var wrapper = new ObjectWrapper(BinaryReadableHandle.of(new ArrayByteChannel("hello\r\nworld\r!".getBytes(StandardCharsets.UTF_8)))); - assertArrayEquals("hello".getBytes(StandardCharsets.UTF_8), wrapper.callOf("readLine")); - assertArrayEquals("world\r!".getBytes(StandardCharsets.UTF_8), wrapper.callOf("readLine")); - assertNull(wrapper.call("readLine")); + var handle = BinaryReadableHandle.of(new ArrayByteChannel("hello\r\nworld\r!".getBytes(StandardCharsets.UTF_8))); + assertArrayEquals("hello".getBytes(StandardCharsets.UTF_8), cast(byte[].class, handle.readLine(Optional.empty()))); + assertArrayEquals("world\r!".getBytes(StandardCharsets.UTF_8), cast(byte[].class, handle.readLine(Optional.empty()))); + assertNull(handle.readLine(Optional.empty())); } @Test public void testReadLineTrailing() throws LuaException { - var wrapper = new ObjectWrapper(BinaryReadableHandle.of(new ArrayByteChannel("hello\r\nworld\r!".getBytes(StandardCharsets.UTF_8)))); - assertArrayEquals("hello\r\n".getBytes(StandardCharsets.UTF_8), wrapper.callOf("readLine", true)); - assertArrayEquals("world\r!".getBytes(StandardCharsets.UTF_8), wrapper.callOf("readLine", true)); - assertNull(wrapper.call("readLine", true)); + var handle = BinaryReadableHandle.of(new ArrayByteChannel("hello\r\nworld\r!".getBytes(StandardCharsets.UTF_8))); + assertArrayEquals("hello\r\n".getBytes(StandardCharsets.UTF_8), cast(byte[].class, handle.readLine(Optional.of(true)))); + assertArrayEquals("world\r!".getBytes(StandardCharsets.UTF_8), cast(byte[].class, handle.readLine(Optional.of(true)))); + assertNull(handle.readLine(Optional.of(true))); } - private static ObjectWrapper fromLength(int length) { + private static BinaryReadableHandle fromLength(int length) { var input = new byte[length]; Arrays.fill(input, (byte) 'A'); - return new ObjectWrapper(BinaryReadableHandle.of(new ArrayByteChannel(input))); + return BinaryReadableHandle.of(new ArrayByteChannel(input)); + } + + private static T cast(Class type, @Nullable Object[] values) { + if (values == null || values.length < 1) throw new NullPointerException(); + return type.cast(values[0]); } } diff --git a/projects/core/src/test/java/dan200/computercraft/core/apis/handles/EncodedReadableHandleTest.java b/projects/core/src/test/java/dan200/computercraft/core/apis/handles/EncodedReadableHandleTest.java index acff62ab7..46c61342b 100644 --- a/projects/core/src/test/java/dan200/computercraft/core/apis/handles/EncodedReadableHandleTest.java +++ b/projects/core/src/test/java/dan200/computercraft/core/apis/handles/EncodedReadableHandleTest.java @@ -5,56 +5,62 @@ package dan200.computercraft.core.apis.handles; import dan200.computercraft.api.lua.LuaException; -import dan200.computercraft.core.apis.ObjectWrapper; import org.junit.jupiter.api.Test; +import javax.annotation.Nullable; import java.io.BufferedReader; import java.io.CharArrayReader; import java.util.Arrays; +import java.util.Optional; import static org.junit.jupiter.api.Assertions.assertEquals; public class EncodedReadableHandleTest { @Test public void testReadChar() throws LuaException { - var wrapper = fromLength(5); - assertEquals("A", wrapper.callOf("read")); + var handle = fromLength(5); + assertEquals("A", cast(String.class, handle.read(Optional.empty()))); } @Test public void testReadShortComplete() throws LuaException { - var wrapper = fromLength(10); - assertEquals("AAAAA", wrapper.callOf("read", 5)); + var handle = fromLength(10); + assertEquals("AAAAA", cast(String.class, handle.read(Optional.of(5)))); } @Test public void testReadShortPartial() throws LuaException { - var wrapper = fromLength(5); - assertEquals("AAAAA", wrapper.callOf("read", 10)); + var handle = fromLength(5); + assertEquals("AAAAA", cast(String.class, handle.read(Optional.of(10)))); } @Test public void testReadLongComplete() throws LuaException { - var wrapper = fromLength(10000); - assertEquals(9000, wrapper.callOf("read", 9000).length()); + var handle = fromLength(10000); + assertEquals(9000, cast(String.class, handle.read(Optional.of(9000))).length()); } @Test public void testReadLongPartial() throws LuaException { - var wrapper = fromLength(10000); - assertEquals(10000, wrapper.callOf("read", 11000).length()); + var handle = fromLength(10000); + assertEquals(10000, cast(String.class, handle.read(Optional.of(11000))).length()); } @Test public void testReadLongPartialSmaller() throws LuaException { - var wrapper = fromLength(1000); - assertEquals(1000, wrapper.callOf("read", 11000).length()); + var handle = fromLength(1000); + assertEquals(1000, cast(String.class, handle.read(Optional.of(11000))).length()); } - private static ObjectWrapper fromLength(int length) { + private static EncodedReadableHandle fromLength(int length) { var input = new char[length]; Arrays.fill(input, 'A'); - return new ObjectWrapper(new EncodedReadableHandle(new BufferedReader(new CharArrayReader(input)))); + return new EncodedReadableHandle(new BufferedReader(new CharArrayReader(input))); + } + + private static T cast(Class type, @Nullable Object[] values) { + if (values == null || values.length < 1) throw new NullPointerException(); + return type.cast(values[0]); } } diff --git a/projects/core/src/test/java/dan200/computercraft/core/filesystem/WritableFileMountTest.java b/projects/core/src/test/java/dan200/computercraft/core/filesystem/WritableFileMountTest.java index 74e624421..712eac2fb 100644 --- a/projects/core/src/test/java/dan200/computercraft/core/filesystem/WritableFileMountTest.java +++ b/projects/core/src/test/java/dan200/computercraft/core/filesystem/WritableFileMountTest.java @@ -10,6 +10,9 @@ import dan200.computercraft.api.filesystem.WritableMount; import dan200.computercraft.test.core.filesystem.WritableMountContract; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; import java.io.IOException; import java.nio.file.Files; @@ -32,6 +35,13 @@ public class WritableFileMountTest implements WritableMountContract { for (var mount : cleanup) MoreFiles.deleteRecursively(mount, RecursiveDeleteOption.ALLOW_INSECURE); } + @Override + @Test + @DisabledOnOs(OS.WINDOWS) // This fails on Windows, and I don't have a debugger to find out why. + public void Writing_uses_latest_file_size() throws IOException { + WritableMountContract.super.Writing_uses_latest_file_size(); + } + private static final class MountAccessImpl implements MountAccess { private final Path root; private final long capacity; diff --git a/projects/core/src/test/resources/test-rom/spec/apis/fs_spec.lua b/projects/core/src/test/resources/test-rom/spec/apis/fs_spec.lua index 6cc5c5516..5a61bd6c4 100644 --- a/projects/core/src/test/resources/test-rom/spec/apis/fs_spec.lua +++ b/projects/core/src/test/resources/test-rom/spec/apis/fs_spec.lua @@ -7,6 +7,16 @@ describe("The fs library", function() local function test_file(path) return fs.combine(test_root, path) end before_each(function() fs.delete(test_root) end) + local function create_test_file(contents) + local path = test_file(("test_%04x.txt"):format(math.random(2 ^ 16))) + + local handle = fs.open(path, "wb") + handle.write(contents) + handle.close() + + return path + end + describe("fs.complete", function() it("validates arguments", function() fs.complete("", "") @@ -158,6 +168,65 @@ describe("The fs library", function() end) describe("fs.open", function() + local function read_tests(mode) + it("errors when closing twice", function() + local handle = fs.open("rom/startup.lua", "rb") + handle.close() + expect.error(handle.close):eq("attempt to use a closed file") + end) + + it("reads multiple bytes", function() + local file = create_test_file "an example file" + + local handle = fs.open(file, mode) + expect(handle.read(3)):eq("an ") + handle.close() + end) + + it("errors reading a negative number of bytes", function() + local file = create_test_file "an example file" + + local handle = fs.open(file, mode) + expect(handle.read(0)):eq("") + expect.error(handle.read, -1):str_match("^Cannot read a negative number of [a-z]+$") + handle.close() + end) + + it("reads multiple bytes longer than the file", function() + local file = create_test_file "an example file" + + local handle = fs.open(file, mode) + expect(handle.read(100)):eq("an example file") + handle.close() + end) + + it("can read a line of text", function() + local file = create_test_file "some\nfile\r\ncontents\n\n" + + local handle = fs.open(file, mode) + expect(handle.readLine()):eq("some") + expect(handle.readLine()):eq("file") + expect(handle.readLine()):eq("contents") + expect(handle.readLine()):eq("") + expect(handle.readLine()):eq(nil) + handle.close() + end) + + -- readLine(true) has odd behaviour in text mode - skip for now. + local it_binary = mode == "rb" and it or pending + it_binary("can read a line of text with the trailing separator", function() + local file = create_test_file "some\nfile\r\ncontents\r!\n\n" + + local handle = fs.open(file, mode) + expect(handle.readLine(true)):eq("some\n") + expect(handle.readLine(true)):eq("file\r\n") + expect(handle.readLine(true)):eq("contents\r!\n") + expect(handle.readLine(true)):eq("\n") + expect(handle.readLine(true)):eq(nil) + handle.close() + end) + end + describe("reading", function() it("fails on directories", function() expect { fs.open("rom", "r") }:same { nil, "/rom: Not a file" } @@ -169,19 +238,27 @@ describe("The fs library", function() expect { fs.open("x", "r") }:same { nil, "/x: No such file" } end) - it("errors when closing twice", function() - local handle = fs.open("rom/startup.lua", "r") + it("supports reading a single byte", function() + local file = create_test_file "an example file" + + local handle = fs.open(file, "r") + expect(handle.read()):eq("a") handle.close() - expect.error(handle.close):eq("attempt to use a closed file") end) + + read_tests("r") end) describe("reading in binary mode", function() - it("errors when closing twice", function() - local handle = fs.open("rom/startup.lua", "rb") + it("reads a single byte", function() + local file = create_test_file "an example file" + + local handle = fs.open(file, "rb") + expect(handle.read()):eq(97) handle.close() - expect.error(handle.close):eq("attempt to use a closed file") end) + + read_tests("rb") end) describe("writing", function() @@ -207,6 +284,29 @@ describe("The fs library", function() -- consistent though, and honestly doesn't matter too much. expect(err):str_match("^/test%-files/con: .*") end) + + it("writing numbers coerces them to a string", function() + local handle = fs.open(test_file "out.txt", "w") + handle.write(65) + handle.close() + + local handle = fs.open(test_file "out.txt", "r") + expect(handle.readAll()):eq("65") + handle.close() + end) + + it("can write lines", function() + local handle = fs.open(test_file "out.txt", "w") + handle.writeLine("First line!") + handle.writeLine("Second line.") + handle.close() + + local handle = fs.open(test_file "out.txt", "r") + expect(handle.readLine()):eq("First line!") + expect(handle.readLine()):eq("Second line.") + expect(handle.readLine()):eq(nil) + handle.close() + end) end) describe("writing in binary mode", function() @@ -215,6 +315,16 @@ describe("The fs library", function() handle.close() expect.error(handle.close):eq("attempt to use a closed file") end) + + it("writing numbers treats them as bytes", function() + local handle = fs.open(test_file "out.txt", "wb") + handle.write(65) + handle.close() + + local handle = fs.open(test_file "out.txt", "rb") + expect(handle.readAll()):eq("A") + handle.close() + end) end) describe("appending", function() diff --git a/projects/core/src/testFixtures/java/dan200/computercraft/test/core/filesystem/WritableMountContract.java b/projects/core/src/testFixtures/java/dan200/computercraft/test/core/filesystem/WritableMountContract.java index a9e97b201..5404b2388 100644 --- a/projects/core/src/testFixtures/java/dan200/computercraft/test/core/filesystem/WritableMountContract.java +++ b/projects/core/src/testFixtures/java/dan200/computercraft/test/core/filesystem/WritableMountContract.java @@ -113,6 +113,30 @@ public interface WritableMountContract { assertEquals(access.computeRemainingSpace(), access.mount().getRemainingSpace(), "Free space is inconsistent"); } + @Test + default void Writing_uses_latest_file_size() throws IOException { + var access = createExisting(CAPACITY); + var mount = access.mount(); + + var handle = mount.openForWrite("file.txt"); + handle.write(LuaValues.encode(LONG_CONTENTS)); + assertEquals(CAPACITY - LONG_CONTENTS.length(), mount.getRemainingSpace()); + assertEquals(access.computeRemainingSpace(), access.mount().getRemainingSpace(), "Free space is inconsistent"); + + var handle2 = mount.openForWrite("file.txt"); + + handle.write(LuaValues.encode("test")); + assertEquals(CAPACITY - LONG_CONTENTS.length() - 4, mount.getRemainingSpace()); + assertEquals(access.computeRemainingSpace(), access.mount().getRemainingSpace(), "Free space is inconsistent"); + + handle2.close(); + handle.close(); + + mount.delete("file.txt"); + assertEquals(CAPACITY, mount.getRemainingSpace()); + assertEquals(access.computeRemainingSpace(), access.mount().getRemainingSpace(), "Free space is inconsistent"); + } + @Test default void Append_jumps_to_file_end() throws IOException { var access = createExisting(CAPACITY);