#6031: Start writing unit tests to cover the desired behaviour of IDe…

…clarationManager::removeDeclaration

include/ideclmanager.h

@@ -137,9 +137,10 @@ class IDeclarationManager :
     // Returns true if the old declaration existed and could successfully be renamed, false on any failure.
     virtual bool renameDeclaration(Type type, const std::string& oldName, const std::string& newName) = 0;
 
-    // Removes the given declaration from the internal dictionaries.
-    // This doesn't remove the declaration from the source files, so this operation
-    // will not survive a reloadDeclarations command. Used to remove temporary decls in edit scenarios.
+    // Removes the given declaration both from memory and the decl file, if there is one.
+    // Only declarations stored in physical files (or unsaved ones) can be removed.
+    // Attempting to remove a read-only declaration (e.g. decls saved in PK4 archives)
+    // will cause a std::logic_error to be thrown.
     virtual void removeDeclaration(Type type, const std::string& name) = 0;
 
     // Re-load all declarations.

test/DeclManager.cpp

@@ -719,6 +719,65 @@ TEST_F(DeclManagerTest, RemoveDeclaration)
     expectDeclIsNotPresent(decl::Type::TestDecl, "decl/precedence_test/1");
 }
 
+// Removing a decl defined in a PK4 file will throw
+TEST_F(DeclManagerTest, RemoveDeclarationInPk4File)
+{
+    GlobalDeclarationManager().registerDeclType("testdecl", std::make_shared<TestDeclarationCreator>());
+    GlobalDeclarationManager().registerDeclFolder(decl::Type::TestDecl, TEST_DECL_FOLDER, ".decl");
+
+    auto decl = GlobalDeclarationManager().findDeclaration(decl::Type::TestDecl, "decl/export/0");
+    EXPECT_TRUE(decl) << "decl/export/0 must be present";
+    EXPECT_FALSE(decl->getBlockSyntax().fileInfo.getIsPhysicalFile()) << "decl/export/0 should be in a PK4 file";
+
+    // Attempting to remove the decl will throw
+    EXPECT_THROW(GlobalDeclarationManager().removeDeclaration(decl->getDeclType(), decl->getDeclName()),
+        std::logic_error) << "Removing a PK4 decl should throw";
+}
+
+// Removing a decl defined in a physical file will succeed
+TEST_F(DeclManagerTest, RemoveDeclarationFromPhysicalFile)
+{
+    GlobalDeclarationManager().registerDeclType("testdecl", std::make_shared<TestDeclarationCreator>());
+    GlobalDeclarationManager().registerDeclFolder(decl::Type::TestDecl, TEST_DECL_FOLDER, ".decl");
+
+    auto decl = GlobalDeclarationManager().findDeclaration(decl::Type::TestDecl, "decl/removal/1");
+    expectDeclIsPresent(decl::Type::TestDecl, "decl/removal/1");
+
+    auto originalSyntax = decl->getBlockSyntax();
+    EXPECT_TRUE(originalSyntax.fileInfo.getIsPhysicalFile()) << "decl/removal/1 must be in a physical file";
+
+    auto fileContents = algorithm::loadTextFromVfsFile(decl->getDeclFilePath());
+    EXPECT_NE(fileContents.find(originalSyntax.contents), std::string::npos) << "Decl source not found";
+
+    // Create a backup copy of the material file we're going to manipulate
+    fs::path declFile = _context.getTestProjectPath() + decl->getDeclFilePath();
+    BackupCopy backup(declFile);
+
+    // Remove the decl, it should remove the source from the file
+    GlobalDeclarationManager().removeDeclaration(decl->getDeclType(), decl->getDeclName());
+
+    auto contentsAfterRemoval = algorithm::loadTextFromVfsFile(decl->getDeclFilePath());
+    EXPECT_NE(contentsAfterRemoval, fileContents) << "File contents should have been changed";
+    EXPECT_EQ(contentsAfterRemoval.find(originalSyntax.contents), std::string::npos) << "Decl source should be gone";
+}
+
+// Removing a decl will also remove leading comment lines
+TEST_F(DeclManagerTest, RemoveDeclarationIncludesLeadingComment)
+{
+    // TODO
+}
+
+// Removing decls with weird line break configurations
+TEST_F(DeclManagerTest, RemoveDeclarationWithNonstandardLinebreaks)
+{
+    // TODO
+}
+
+TEST_F(DeclManagerTest, RemoveUnsavedDeclaration)
+{
+    // TODO
+}
+
 TEST_F(DeclManagerTest, RenameDeclaration)
 {
     GlobalDeclarationManager().registerDeclType("testdecl", std::make_shared<TestDeclarationCreator>());

test/resources/tdm/testdecls/removal_tests.decl

@@ -0,0 +1,42 @@
+// Test declarations used for some DeclarationManager unit tests
+// Don't even alter the whitespace in this file.
+// Comments at the beginning of the file with an empty line in between
+
+decl/removal/0
+{
+    diffusemap textures/removal/0
+}
+
+/* Without "testdecl" typename, C-style comment */
+decl/removal/1
+{
+    diffusemap textures/removal/1
+}
+// Some comments after decl/removal/1
+
+/**
+ A multiline comment before this decl
+ */
+testdecl decl/removal/2
+{
+    diffusemap textures/removal/2
+}
+// Some comments after decl/removal/2
+
+// CamelCase typename shouldn't make a difference
+TestDecl decl/removal/3
+{
+    diffusemap textures/removal/3
+}
+
+// Comment with a line before the start of the decl decl/removal/5
+
+testdecl decl/removal/5 { // a comment in the same line as the opening brace
+    diffusemap textures/removal/5
+}
+
+// Everything in the same line
+testdecl decl/removal/6 { diffusemap textures/removal/6 }
+testdecl decl/removal/7 { diffusemap textures/removal/6 }
+
+testdecl decl/removal/8 { diffusemap textures/removal/6 }