From 2e600efa22b012f60949b36649fddc67466f04f3 Mon Sep 17 00:00:00 2001 From: Brett Groshong Date: Thu, 2 Feb 2023 09:02:21 -0800 Subject: [PATCH 1/4] Bring back test : Generation TestGraphAsIs --- .../Tests/Generation/GenerationTests.cs | 54 +++++++++---------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/com.unity.sg2/Tests/Generation/GenerationTests.cs b/com.unity.sg2/Tests/Generation/GenerationTests.cs index 88f6858d9ec..aefe6761936 100644 --- a/com.unity.sg2/Tests/Generation/GenerationTests.cs +++ b/com.unity.sg2/Tests/Generation/GenerationTests.cs @@ -86,35 +86,31 @@ private static Texture2D DrawToTex(Material material, int width = 4, int height } - // TODO (Brett) This is disabled because of Generation errors with the Dec. - // TODO (Brett) branch update. - // TODO (Brett) Re-enable ASAP. - - // static object[] testAsIsSource = - // { - // ("Add1", new Color(1,0,0,1)), //Colors with Alpha 1 since target is opaque - // ("Add2", new Color(0,1,0,1)), - // ("Add3", new Color(1,1,0,1)), - // }; - // - // [Test] - // [TestCaseSource("testAsIsSource")] - // public static void TestGraphAsIs((string nodeToCompile, Color expectedColor) input) - // { - // var shaderString = Interpreter.GetShaderForNode(graph.GetNodeReader(input.nodeToCompile), graph, registry, out _); - // var shader = MakeShader(shaderString); - // var rt = DrawToTex(shader); - // try - // { - // var pixelColor = rt.GetPixel(0,0); - // Assert.AreEqual(pixelColor, input.expectedColor); - // } - // catch(Exception e) - // { - // File.WriteAllBytes($"Assets/FailureImage{input.nodeToCompile}.jpg", rt.EncodeToJPG()); - // throw e; - // } - // } + static object[] testAsIsSource = + { + ("Add1", new Color(1,0,0,1)), //Colors with Alpha 1 since target is opaque + ("Add2", new Color(0,1,0,1)), + ("Add3", new Color(1,1,0,1)), + }; + + [Test] + [TestCaseSource("testAsIsSource")] + public static void TestGraphAsIs((string nodeToCompile, Color expectedColor) input) + { + var shaderString = Interpreter.GetShaderForNode(graph.GetNodeReader(input.nodeToCompile), graph, registry, out _); + var shader = MakeShader(shaderString); + var rt = DrawToTex(shader); + try + { + var pixelColor = rt.GetPixel(0,0); + Assert.AreEqual(pixelColor, input.expectedColor); + } + catch(Exception e) + { + File.WriteAllBytes($"Assets/FailureImage{input.nodeToCompile}.jpg", rt.EncodeToJPG()); + throw e; + } + } [Test] public static void TestGraphReferenceNode() From 05b3a71e102222397eb19b359001339bd6a0579c Mon Sep 17 00:00:00 2001 From: Brett Groshong Date: Thu, 2 Feb 2023 10:40:54 -0800 Subject: [PATCH 2/4] Ignore CLDS.TestCopyPaste for 2 sprints. --- .../ContextLayeredDataStorageTests.cs | 145 +++++++++--------- 1 file changed, 71 insertions(+), 74 deletions(-) diff --git a/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs b/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs index 5441337d292..e1927586090 100644 --- a/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs +++ b/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs @@ -517,80 +517,77 @@ public void IsSubPathWorks() } } } + + [Ignore("GSG-1614", Until="2023-03-15")] + [Test] + public void TestCopyPaste() + { + ContextLayeredDataStorage clds = new ContextLayeredDataStorage(); + clds.AddData("a"); + clds.AddData("a.b", 13); + clds.AddData("a.b.c", 35.4f); + clds.AddData("a.b.c.d.e", true); + clds.SetMetadata("a.b", "foo", new Color(1, 0, 0)); - // TODO (Brett) This is commented out to bring tests to a passing status. - // TODO (Brett) This test was not removed because it is indicating a valuable failure - // TODO (Brett) that should be addressed. - - // [Test] - // public void TestCopyPaste() - // { - // ContextLayeredDataStorage clds = new ContextLayeredDataStorage(); - // clds.AddData("a"); - // clds.AddData("a.b", 13); - // clds.AddData("a.b.c", 35.4f); - // clds.AddData("a.b.c.d.e", true); - // clds.SetMetadata("a.b", "foo", new Color(1, 0, 0)); - // - // List elements = new List() - // { - // clds.Search("a"), - // clds.Search("a.b"), - // clds.Search("a.b.c"), - // clds.Search("a.b.c.d.e") - // }; - // - // var ser = clds.CopyElementCollection(elements); - // - // //Paste into same CLDS - // clds.PasteElementCollection(ser.layer, ser.metadata, "Root", out _); - // var copied = clds.Search("a_1"); - // Assert.NotNull(copied); - // Assert.IsTrue(copied.GetChildren().Count() == 1); - // copied = copied.GetChild("b"); - // Assert.NotNull(copied); - // Assert.AreEqual(13, copied.GetData()); - // Assert.AreEqual(new Color(1, 0, 0), clds.GetMetadata(copied.Element.ID, "foo")); - // Assert.IsTrue(copied.GetChildren().Count() == 1); - // copied = copied.GetChild("c"); - // Assert.NotNull(copied); - // Assert.AreEqual(35.4f, copied.GetData()); - // //Okay, so this is a weird one and I want to explain it, since either myself in 3 months or - // //whoever is looking at this code later might think this is a bug. Of course "c" has a child, - // //its "d.e"! But what needs to be remembered is we act on the flat structure, and "GetChildren" - // //will only return the _immediate_ children of a reader (at least in the base DataReader case). - // //That means, if "d" were contained in this structure, it could be returned. But since its not - // //here, and "d.e" wont be seen as an immediate child, its ignored. - // Assert.AreEqual(0, copied.GetChildren().Count()); - // //This part though seems to go against that; "c" has no children, why can you getchild on "d.e" - // //and get a correct value? Currently, getchild just appends the rest of the localID onto c's - // //full path ID, and then just searches the graph for that, which will search for "a.b.c.d.e" - // //and find the correct value. The name could potentially be changed, but thats really up to - // //how the reader is implemented. - // copied = copied.GetChild("d.e"); - // Assert.NotNull(copied); - // Assert.AreEqual(true, copied.GetData()); - // Assert.IsTrue(copied.GetChildren().Count() == 0); - // - // //Paste into new CLDS - // clds = new ContextLayeredDataStorage(); - // clds.PasteElementCollection(ser.layer, ser.metadata, "Root", out _); - // copied = clds.Search("a"); - // Assert.NotNull(copied); - // Assert.IsTrue(copied.GetChildren().Count() == 1); - // copied = copied.GetChild("b"); - // Assert.NotNull(copied); - // Assert.AreEqual(13, copied.GetData()); - // Assert.AreEqual(new Color(1, 0, 0), clds.GetMetadata(copied.Element.ID, "foo")); - // Assert.IsTrue(copied.GetChildren().Count() == 1); - // copied = copied.GetChild("c"); - // Assert.NotNull(copied); - // Assert.AreEqual(35.4f, copied.GetData()); - // Assert.AreEqual(0, copied.GetChildren().Count()); - // copied = copied.GetChild("d.e"); - // Assert.NotNull(copied); - // Assert.AreEqual(true, copied.GetData()); - // Assert.IsTrue(copied.GetChildren().Count() == 0); - // } + List elements = new List() + { + clds.Search("a"), + clds.Search("a.b"), + clds.Search("a.b.c"), + clds.Search("a.b.c.d.e") + }; + + var ser = clds.CopyElementCollection(elements); + + //Paste into same CLDS + clds.PasteElementCollection(ser.layer, ser.metadata, "Root", out _); + var copied = clds.Search("a_1"); + Assert.NotNull(copied); + Assert.IsTrue(copied.GetChildren().Count() == 1); + copied = copied.GetChild("b"); + Assert.NotNull(copied); + Assert.AreEqual(13, copied.GetData()); + Assert.AreEqual(new Color(1, 0, 0), clds.GetMetadata(copied.Element.ID, "foo")); + Assert.IsTrue(copied.GetChildren().Count() == 1); + copied = copied.GetChild("c"); + Assert.NotNull(copied); + Assert.AreEqual(35.4f, copied.GetData()); + //Okay, so this is a weird one and I want to explain it, since either myself in 3 months or + //whoever is looking at this code later might think this is a bug. Of course "c" has a child, + //its "d.e"! But what needs to be remembered is we act on the flat structure, and "GetChildren" + //will only return the _immediate_ children of a reader (at least in the base DataReader case). + //That means, if "d" were contained in this structure, it could be returned. But since its not + //here, and "d.e" wont be seen as an immediate child, its ignored. + Assert.AreEqual(0, copied.GetChildren().Count()); + //This part though seems to go against that; "c" has no children, why can you getchild on "d.e" + //and get a correct value? Currently, getchild just appends the rest of the localID onto c's + //full path ID, and then just searches the graph for that, which will search for "a.b.c.d.e" + //and find the correct value. The name could potentially be changed, but thats really up to + //how the reader is implemented. + copied = copied.GetChild("d.e"); + Assert.NotNull(copied); + Assert.AreEqual(true, copied.GetData()); + Assert.IsTrue(copied.GetChildren().Count() == 0); + + //Paste into new CLDS + clds = new ContextLayeredDataStorage(); + clds.PasteElementCollection(ser.layer, ser.metadata, "Root", out _); + copied = clds.Search("a"); + Assert.NotNull(copied); + Assert.IsTrue(copied.GetChildren().Count() == 1); + copied = copied.GetChild("b"); + Assert.NotNull(copied); + Assert.AreEqual(13, copied.GetData()); + Assert.AreEqual(new Color(1, 0, 0), clds.GetMetadata(copied.Element.ID, "foo")); + Assert.IsTrue(copied.GetChildren().Count() == 1); + copied = copied.GetChild("c"); + Assert.NotNull(copied); + Assert.AreEqual(35.4f, copied.GetData()); + Assert.AreEqual(0, copied.GetChildren().Count()); + copied = copied.GetChild("d.e"); + Assert.NotNull(copied); + Assert.AreEqual(true, copied.GetData()); + Assert.IsTrue(copied.GetChildren().Count() == 0); + } } } From b178989f31f25b59062865511e7edf8010e95806 Mon Sep 17 00:00:00 2001 From: Brett Groshong Date: Thu, 2 Feb 2023 10:45:22 -0800 Subject: [PATCH 3/4] Delete incorrect / stale searcher tests. --- .../Tests/GraphUI/DataModel/Searcher.meta | 8 --- ...ShaderGraphSearcherDatabaseProviderTest.cs | 56 ------------------- ...rGraphSearcherDatabaseProviderTest.cs.meta | 11 ---- 3 files changed, 75 deletions(-) delete mode 100644 com.unity.sg2/Tests/GraphUI/DataModel/Searcher.meta delete mode 100644 com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs delete mode 100644 com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs.meta diff --git a/com.unity.sg2/Tests/GraphUI/DataModel/Searcher.meta b/com.unity.sg2/Tests/GraphUI/DataModel/Searcher.meta deleted file mode 100644 index eccc217e987..00000000000 --- a/com.unity.sg2/Tests/GraphUI/DataModel/Searcher.meta +++ /dev/null @@ -1,8 +0,0 @@ -fileFormatVersion: 2 -guid: 941b8a765220e4dcf9525129f23c8619 -folderAsset: yes -DefaultImporter: - externalObjects: {} - userData: - assetBundleName: - assetBundleVariant: diff --git a/com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs b/com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs deleted file mode 100644 index 7424408f390..00000000000 --- a/com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs +++ /dev/null @@ -1,56 +0,0 @@ -using NUnit.Framework; -using Unity.ItemLibrary.Editor; -using UnityEditor.ShaderGraph.GraphDelta; - -namespace UnityEditor.ShaderGraph.GraphUI.UnitTests -{ - [TestFixture] - class ShaderGraphSearcherDatabaseProviderTest - { - // TODO (Brett) Correct this test once a testable registry can be created. - //[Test] - public void GetNodeSearcherItems_Basic() - { - // make a registry - // load some node descriptors - // make a mock shader graph model - // get the list of node seacher items - // check that the display names are in the list as searcher item names - // make sure that a ItemLibraryDatabase can be made - } - - // TODO (Brett) This test is currently not being run as part of the suite - // because the "empty" registry is actually filled with 207 keys. - // TODO (Brett) Make this test correct. - //[Test] - public void GetNodeSearcherItems_WithEmptyRegistry() - { - // Setup - ShaderGraphRegistry registry = new ShaderGraphRegistry(); - SGGraphModel graphModel = new SGGraphModelMock(registry); - - // Test ItemLibraryItem list from an empty registry - var searcherItems = ShaderGraphSearcherDatabaseProvider.GetNodeSearcherItems(graphModel); - Assert.Zero(searcherItems.Count, "ItemLibraryItem list created from an empty Registry should be empty"); - - - // Test ItemLibraryDatabase creation from empty ItemLibraryItem list - Assert.DoesNotThrow(() => - { - ItemLibraryDatabase db = new(searcherItems); - }, "Should be able to create a ItemLibraryDatabase from an empty ItemLibraryItem list"); - } - - // TODO (Brett) Correct this test once a testable registry can be created. - //[Test] - public void GetNodeSearcherItems_WithDuplicates() - { - // make a registry - // add multiple node descriptors with the same display name - // make a mock shader graph model - // get the list of node seacher items - // check that the duplicated dispaly name only appears in one searcher item - // make sure that a ItemLibraryDatabase can be made - } - } -} diff --git a/com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs.meta b/com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs.meta deleted file mode 100644 index 473554b5816..00000000000 --- a/com.unity.sg2/Tests/GraphUI/DataModel/Searcher/ShaderGraphSearcherDatabaseProviderTest.cs.meta +++ /dev/null @@ -1,11 +0,0 @@ -fileFormatVersion: 2 -guid: d262b37fcbfa64b3aa0e1aae6163650a -MonoImporter: - externalObjects: {} - serializedVersion: 2 - defaultReferences: [] - executionOrder: 0 - icon: {instanceID: 0} - userData: - assetBundleName: - assetBundleVariant: From 5644c36ba6165b4492a4049e477c6ddf5a504ed0 Mon Sep 17 00:00:00 2001 From: Brett Groshong Date: Fri, 3 Feb 2023 12:12:32 -0800 Subject: [PATCH 4/4] WIP --- .../ContextLayeredDataStorageTests.cs | 2 +- com.unity.sg2/Tests/GraphUI/GraphEdgeTests.cs | 41 +++++++++---------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs b/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs index e1927586090..2c7e8aa8621 100644 --- a/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs +++ b/com.unity.sg2/Tests/ContextLayeredDataStorage/ContextLayeredDataStorageTests.cs @@ -517,7 +517,7 @@ public void IsSubPathWorks() } } } - + [Ignore("GSG-1614", Until="2023-03-15")] [Test] public void TestCopyPaste() diff --git a/com.unity.sg2/Tests/GraphUI/GraphEdgeTests.cs b/com.unity.sg2/Tests/GraphUI/GraphEdgeTests.cs index 3c0e887c94f..fae281b0601 100644 --- a/com.unity.sg2/Tests/GraphUI/GraphEdgeTests.cs +++ b/com.unity.sg2/Tests/GraphUI/GraphEdgeTests.cs @@ -13,27 +13,26 @@ class GraphEdgeTests : BaseGraphWindowTest /// protected override GraphInstantiation GraphToInstantiate => GraphInstantiation.MemoryBlank; - // TODO (Brett) This is commented out to bring tests to a passing status. - // TODO (Brett) This test was not removed because it is indicating a valuable failure - // TODO (Brett) that should be addressed. + [UnityTest] + public IEnumerator TestEdgeCanBeDeleted() + { + // create and connect two nodes + var addNodeModel = SGGraphTestUtils.CreateNodeByName(GraphModel,"Add", Vector2.zero); + Assert.NotNull(addNodeModel, "Add node could not be added to the graph"); + var previewNodeModel = SGGraphTestUtils.CreateNodeByName(GraphModel, "Preview", Vector2.zero); + Assert.NotNull(previewNodeModel, "Preview node model could not be added to the graph"); - // [UnityTest] - // public IEnumerator TestEdgeCanBeDeleted() - // { - // // Set up the graph - // yield return m_TestInteractionHelper.CreateNodesAndConnect(); - // - // var edgeModel = m_MainWindow.GetEdgeModelFromGraphByName("Add", "Preview"); - // - // // Select element programmatically because it might be behind another one - // m_GraphView.Dispatch(new SelectElementsCommand(SelectElementsCommand.SelectionMode.Replace, edgeModel)); - // yield return null; - // - // Assert.IsTrue(m_TestEventHelper.SendDeleteCommand()); - // yield return null; - // - // edgeModel = m_MainWindow.GetEdgeModelFromGraphByName("Add", "Preview"); - // Assert.IsNull(edgeModel, "Edge should be null after delete operation"); - // } + var edgeModel = m_MainWindow.GetEdgeModelFromGraphByName("Add", "Preview"); + + // Select element programmatically because it might be behind another one + m_GraphView.Dispatch(new SelectElementsCommand(SelectElementsCommand.SelectionMode.Replace, edgeModel)); + yield return null; + + Assert.IsTrue(m_TestEventHelper.SendDeleteCommand()); + yield return null; + + edgeModel = m_MainWindow.GetEdgeModelFromGraphByName("Add", "Preview"); + Assert.IsNull(edgeModel, "Edge should be null after delete operation"); + } } }