From accf72a0b384b9c5b39e3b4c505e2f7f3b27d018 Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 05:30:26 -0600 Subject: [PATCH 1/9] Create fewer test files in `DirectoryDoesNotExist` test The constructor being tested does not branch differently based on relative path prefixes. There doesn't seem to be a clear purpose in testing this code with various prefixes. Maybe the lower level path handling code could use tests with various prefixes, though if needed that should get it's own tests rather than be a part of this one. --- OP2UtilityTest/Stream/FileWriter.test.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index e67b1ed5..fa83b052 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -84,8 +84,6 @@ TEST(FileWriter, InvalidFilename) { } TEST(FileWriter, DirectoryDoesNotExist) { - WriteToNewDirectory("../NewDirectory/TestFile.temp"); - WriteToNewDirectory("./NewDirectory/TestFile.temp"); WriteToNewDirectory("NewDirectory/TestFile.temp"); } From bf0175d5969d7c08484c1d0942e67556111af2f3 Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 05:39:45 -0600 Subject: [PATCH 2/9] Inline and remove now single use `WriteToNewDirectory` helper Actually, as a matter of style, I don't know how common it is to put unit test assertions in helper methods, as opposed to keeping all test assertions in the test case itself. Something about it seemed mildly unexpected, though I can see why it would be convenient. --- OP2UtilityTest/Stream/FileWriter.test.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index fa83b052..d47ced3a 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -8,7 +8,6 @@ using namespace OP2Utility; -void WriteToNewDirectory(const std::string& path); TEST(FileWriterOpenMode, BadFlagCombinations) { using OpenMode = Stream::FileWriter::OpenMode; @@ -84,17 +83,14 @@ TEST(FileWriter, InvalidFilename) { } TEST(FileWriter, DirectoryDoesNotExist) { - WriteToNewDirectory("NewDirectory/TestFile.temp"); -} + const std::string& path("NewDirectory/TestFile.temp"); -// Will delete the path after testing creation -void WriteToNewDirectory(const std::string& path) -{ // New directory should not be created when writer cannot create new files EXPECT_THROW(Stream::FileWriter writer(path, Stream::FileWriter::OpenMode::CanOpenExisting), std::runtime_error); EXPECT_FALSE(XFile::PathExists(path)); EXPECT_NO_THROW(Stream::FileWriter writer(path)); + // Delete the path after testing creation XFile::DeletePath(path); } From d00c9a4148475d814491aa80722ef37e9d5b691b Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 05:44:27 -0600 Subject: [PATCH 3/9] Split success and failure test cases These are effectively two separate behaviors, that should probably get two separate tests. --- OP2UtilityTest/Stream/FileWriter.test.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index d47ced3a..3c1a0548 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -82,12 +82,16 @@ TEST(FileWriter, InvalidFilename) { EXPECT_THROW(Stream::FileWriter fileWriter("data"), std::runtime_error); } -TEST(FileWriter, DirectoryDoesNotExist) { +TEST(FileWriter, DirectoryDoesNotExistFail) { const std::string& path("NewDirectory/TestFile.temp"); // New directory should not be created when writer cannot create new files EXPECT_THROW(Stream::FileWriter writer(path, Stream::FileWriter::OpenMode::CanOpenExisting), std::runtime_error); EXPECT_FALSE(XFile::PathExists(path)); +} + +TEST(FileWriter, DirectoryDoesNotExistCreate) { + const std::string& path("NewDirectory/TestFile.temp"); EXPECT_NO_THROW(Stream::FileWriter writer(path)); From 39d66bf3b7d23e8c9bd8dc909a9db9d3b4dd5fd9 Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 05:54:40 -0600 Subject: [PATCH 4/9] Add assertion for file existing in create test --- OP2UtilityTest/Stream/FileWriter.test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index 3c1a0548..81e16967 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -94,6 +94,7 @@ TEST(FileWriter, DirectoryDoesNotExistCreate) { const std::string& path("NewDirectory/TestFile.temp"); EXPECT_NO_THROW(Stream::FileWriter writer(path)); + EXPECT_TRUE(XFile::PathExists(path)); // Delete the path after testing creation XFile::DeletePath(path); From f76940c0be551d221d8de2826c11524ae8863eea Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 06:01:15 -0600 Subject: [PATCH 5/9] Use a different folder name between test cases We don't want coupling between independent tests. We could check that a folder is not created in the failure test case, though if the create test case leaves one around, that could cause the failure test case to fail due to different tests on the same object. --- OP2UtilityTest/Stream/FileWriter.test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index 81e16967..d4697d5b 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -83,7 +83,7 @@ TEST(FileWriter, InvalidFilename) { } TEST(FileWriter, DirectoryDoesNotExistFail) { - const std::string& path("NewDirectory/TestFile.temp"); + const std::string& path("NewDirectoryNotCreated/TestFile.temp"); // New directory should not be created when writer cannot create new files EXPECT_THROW(Stream::FileWriter writer(path, Stream::FileWriter::OpenMode::CanOpenExisting), std::runtime_error); From 4158a1f00e2152224862043d15ef3afde5f125db Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 06:04:21 -0600 Subject: [PATCH 6/9] Test for existence/non-existence of folder in test cases --- OP2UtilityTest/Stream/FileWriter.test.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index d4697d5b..8b66b4fb 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -83,17 +83,21 @@ TEST(FileWriter, InvalidFilename) { } TEST(FileWriter, DirectoryDoesNotExistFail) { - const std::string& path("NewDirectoryNotCreated/TestFile.temp"); + const std::string folder{"NewDirectoryNotCreated/"}; + const std::string path{folder + "TestFile.temp"}; // New directory should not be created when writer cannot create new files EXPECT_THROW(Stream::FileWriter writer(path, Stream::FileWriter::OpenMode::CanOpenExisting), std::runtime_error); + EXPECT_FALSE(XFile::PathExists(folder)); EXPECT_FALSE(XFile::PathExists(path)); } TEST(FileWriter, DirectoryDoesNotExistCreate) { - const std::string& path("NewDirectory/TestFile.temp"); + const std::string folder{"NewDirectory/"}; + const std::string path{folder + "TestFile.temp"}; EXPECT_NO_THROW(Stream::FileWriter writer(path)); + EXPECT_TRUE(XFile::PathExists(folder)); EXPECT_TRUE(XFile::PathExists(path)); // Delete the path after testing creation From 0a83706dc39fca06ff4e935ac0200bd8893fece0 Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 06:26:15 -0600 Subject: [PATCH 7/9] Cleanup temp folder as well as the file --- OP2UtilityTest/Stream/FileWriter.test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index 8b66b4fb..8cac0bf1 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -102,4 +102,5 @@ TEST(FileWriter, DirectoryDoesNotExistCreate) { // Delete the path after testing creation XFile::DeletePath(path); + XFile::DeletePath(folder); } From ee68db0014e52ef6514e78ea40c6a4275de9a4ea Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 07:35:34 -0600 Subject: [PATCH 8/9] Cleanup folder in `PermissionChecks` test case The lack of cleanup was hidden by the cleanup in the recently updated test case. Whether a folder was left behind or not was potentially order dependent. This is because both test cases operated on the same folder, so effectively shared an object. --- OP2UtilityTest/Stream/FileWriter.test.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index 8cac0bf1..92741dba 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -57,12 +57,14 @@ TEST(FileWriterOpenMode, PermissionChecks) { ASSERT_THROW(Stream::FileWriter writer(filename, OpenMode::CanOpenNew), std::runtime_error); // Try to open new file in new directory with permission - const std::string directoryAndFilename("NewDirectory/OpenModePermissionChecks.temp"); + const std::string folder{"NewDirectory/"}; + const std::string directoryAndFilename(folder + "OpenModePermissionChecks.temp"); ASSERT_NO_THROW(Stream::FileWriter writer(directoryAndFilename, OpenMode::CanOpenNew)); // Cleanup temporary file XFile::DeletePath(filename); XFile::DeletePath(directoryAndFilename); + XFile::DeletePath(folder); } From b85782db55528c3679f899a5c0739db7ed3266a8 Mon Sep 17 00:00:00 2001 From: Daniel Stevens Date: Mon, 10 Aug 2026 07:40:40 -0600 Subject: [PATCH 9/9] Rename created folders to make tests independent Potentially this could also be useful if tests are run in parallel, as using separate folders would prevent them from interfering with each other. --- OP2UtilityTest/Stream/FileWriter.test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/OP2UtilityTest/Stream/FileWriter.test.cpp b/OP2UtilityTest/Stream/FileWriter.test.cpp index 92741dba..b9fe4c67 100644 --- a/OP2UtilityTest/Stream/FileWriter.test.cpp +++ b/OP2UtilityTest/Stream/FileWriter.test.cpp @@ -57,7 +57,7 @@ TEST(FileWriterOpenMode, PermissionChecks) { ASSERT_THROW(Stream::FileWriter writer(filename, OpenMode::CanOpenNew), std::runtime_error); // Try to open new file in new directory with permission - const std::string folder{"NewDirectory/"}; + const std::string folder{"NewDirectoryPermissionChecks/"}; const std::string directoryAndFilename(folder + "OpenModePermissionChecks.temp"); ASSERT_NO_THROW(Stream::FileWriter writer(directoryAndFilename, OpenMode::CanOpenNew)); @@ -95,7 +95,7 @@ TEST(FileWriter, DirectoryDoesNotExistFail) { } TEST(FileWriter, DirectoryDoesNotExistCreate) { - const std::string folder{"NewDirectory/"}; + const std::string folder{"NewDirectoryCreated/"}; const std::string path{folder + "TestFile.temp"}; EXPECT_NO_THROW(Stream::FileWriter writer(path));