[USDU-407] Add automated tests for Analytics #398

lee-aandrew · 2023-07-10T01:55:59Z

Purpose of this PR

Analytics have been added for our USD package giving us various information about the usage of the package
We thought it would have be needed to have some automated test cases for the analytics

Ticket/Jira #:
[USDU-407]

Testing

Functional Testing status:
Adds functional testcases for the analytics for the following parts:

Import
Export
Re-import (refresh / update)

Could not add planned Recorder analytics automated testing as a lot of recorder aspect is private / internal, and unless I use reflections I cannot instantiate a lot of the classes that I would need to set up the test
LMK if you guys think its needed

Performance Testing status:

Overall Product Risks

Complexity:

Halo Effect:

Additional information

Note to reviewers:

Reminder:

Add entry in CHANGELOG.md (if applicable)

SimonBoorer · 2023-07-10T15:37:12Z

Are you aware the tests are failing on 2023.1 and newer?

Library\PackageCache\com.unity.formats.usd.tests@090deec1ff8c\Tests\Editor\EditorAnalyticsBaseFixture.cs(8,7): error CS0246: The type or namespace name 'Newtonsoft' could not be found (are you missing a using directive or an assembly reference?)

… version to be 2023.2

michaeljblain

Some suggestions. Looks like tests are still failing on trunk.

package/com.unity.formats.usd/Tests/Common/BaseFixture.cs

michaeljblain · 2023-07-10T16:49:02Z

package/com.unity.formats.usd/Tests/Common/BaseFixture.cs

+                }
+                catch
+                {
+                    Debug.Log("Artifact Clean up has failed - This should not happen in most cases, but even if so, the test case should not be affected.");


Can we feed back further detail on the failure by also logging the exception?

package/com.unity.formats.usd/Tests/Editor/EditorAnalyticsBaseFixture.cs

…om the manifest.json | Adds analytics to the formats.usd package

lee-aandrew · 2023-07-25T13:03:58Z

~~Stuck due to problem on the analytics side - Mike Stowell is looking into the issue~~

Requires Mike's fixes: Link

… version to be 2023.2

…om the manifest.json | Adds analytics to the formats.usd package

…ies/usd-unity-sdk into AddAnalyticsTest # Conflicts: # package/com.unity.formats.usd/Tests/Common/BaseFixture.cs # package/com.unity.formats.usd/Tests/Common/Data/Material/Invalid.format # package/com.unity.formats.usd/Tests/Common/Data/Material/Invalid.format.meta # package/com.unity.formats.usd/Tests/Common/Data/Material/InvalidContent.usda # package/com.unity.formats.usd/Tests/Common/Data/Material/InvalidContent.usda.meta # package/com.unity.formats.usd/Tests/Common/Data/Material/Kiki_Blendshape.usd # package/com.unity.formats.usd/Tests/Common/Data/Material/Kiki_Blendshape.usd.meta # package/com.unity.formats.usd/Tests/Editor/EditorAnalyticsBaseFixture.cs # package/com.unity.formats.usd/Tests/Editor/EditorExportAnalyticsTests.cs # package/com.unity.formats.usd/Tests/Editor/EditorExportAnalyticsTests.cs.meta # package/com.unity.formats.usd/Tests/Editor/EditorImportAnalyticsTests.cs # package/com.unity.formats.usd/Tests/Editor/EditorImportAnalyticsTests.cs.meta # package/com.unity.formats.usd/Tests/Editor/EditorReImportAnalyticsTests.cs # package/com.unity.formats.usd/Tests/Editor/EditorReImportAnalyticsTests.cs.meta # package/com.unity.formats.usd/Tests/Editor/Unity.Formats.USD.Tests.Editor.asmdef

lee-aandrew · 2023-07-27T13:30:07Z

.yamato/package-test.yml

@@ -13,7 +13,11 @@ test_{{ platform.name }}_{{ editor.version }}:
    UPMCI_PKG: "upm-ci-utils@stable"
  commands:
      - npm install {% if platform.name == "win" %}"%UPMCI_PKG%"{% else %}"$UPMCI_PKG"{% endif %} -g --registry {{ registry.npm }}
-      - upm-ci package test -u {{ editor.version }} --package-path package/com.unity.formats.usd --type package-tests --extra-utr-arg="--api-profile=NET_4_6"
+      {% if editor.version == "trunk" %}
+        - upm-ci package test -u {{ editor.version }} --package-path package/com.unity.formats.usd --type package-tests --extra-utr-arg="--api-profile=NET_4_6" --extra-editor-arg=-editorAnalyticsTestMode        


This is behind an if statement as there was a bug in the analytics side where if this was set for versions less than trunk, it would crash the editor

I don't think we should specify 'trunk' here as it isn't robust for the future. Eventually trunk will be newer that 2023 and the '23 LTS won't be tested/ the tests will run without this editor arg being set. I'm not sure what the correct answer is though :)

vickycl

Small fixes and clean up suggested, but otherwise good so far!

vickycl · 2023-07-11T15:34:04Z

TestProject/Usd-Development/Packages/manifest.json

@@ -1,11 +1,12 @@
 {
  "dependencies": {
    "com.unity.analytics": "3.5.3",
+    "com.unity.editor-analytics-debugger": "0.1.0",


Is this ok in 2019.4?

Oops, this is leftover from when I initially started the review 😂

vickycl · 2023-07-27T13:49:08Z

.yamato/package-test.yml

@@ -13,7 +13,11 @@ test_{{ platform.name }}_{{ editor.version }}:
    UPMCI_PKG: "upm-ci-utils@stable"
  commands:
      - npm install {% if platform.name == "win" %}"%UPMCI_PKG%"{% else %}"$UPMCI_PKG"{% endif %} -g --registry {{ registry.npm }}
-      - upm-ci package test -u {{ editor.version }} --package-path package/com.unity.formats.usd --type package-tests --extra-utr-arg="--api-profile=NET_4_6"
+      {% if editor.version == "trunk" %}
+        - upm-ci package test -u {{ editor.version }} --package-path package/com.unity.formats.usd --type package-tests --extra-utr-arg="--api-profile=NET_4_6" --extra-editor-arg=-editorAnalyticsTestMode        


I don't think we should specify 'trunk' here as it isn't robust for the future. Eventually trunk will be newer that 2023 and the '23 LTS won't be tested/ the tests will run without this editor arg being set. I'm not sure what the correct answer is though :)

vickycl · 2023-07-27T13:50:04Z

.yamato/package-test.yml

@@ -13,7 +13,11 @@ test_{{ platform.name }}_{{ editor.version }}:
    UPMCI_PKG: "upm-ci-utils@stable"
  commands:
      - npm install {% if platform.name == "win" %}"%UPMCI_PKG%"{% else %}"$UPMCI_PKG"{% endif %} -g --registry {{ registry.npm }}
-      - upm-ci package test -u {{ editor.version }} --package-path package/com.unity.formats.usd --type package-tests --extra-utr-arg="--api-profile=NET_4_6"
+      {% if editor.version == "trunk" %}
+        - upm-ci package test -u {{ editor.version }} --package-path package/com.unity.formats.usd --type package-tests --extra-utr-arg="--api-profile=NET_4_6" --extra-editor-arg=-editorAnalyticsTestMode        


does the --extra-editor-arg=-editorAnalyticsTestMode alleviate the need for UNITY_THISISABUILDMACHINE? ie does it set the runmode to test?

I realized i set the "UNITY_THISISABUILDMACHINE" but it never got passed over to the yamato call, and i believe our test machines already have the "UNITY_THISISABUILDMACHINE" set to true

vickycl · 2023-07-27T13:51:54Z

package/com.unity.formats.usd/Tests/Common/Data/Invalid/Kiki_Blendshape.usd

I can't see what is included here, but it's quite a big asset to include for the sake of a test, and I'm also not sure whether it can be shared publicly in this way?

Thats a good point, i just needed an invalid USD file for a failure, in this case blendshapes, i'll look for another asset

vickycl · 2023-07-27T13:53:44Z

package/com.unity.formats.usd/Tests/Editor/EditorAnalyticsBaseFixture.cs

+            InitUsd.Initialize();
+        }
+
+        public IEnumerator WaitForUsdAnalytics<T>(string expectedType, System.Action<AnalyticsEvent> expectedEvent, float attemptFrameCountLimit = 300) where T: UsdAnalyticsEvent


This frame count limit is very high- are you sure it needs to be this high?

Changed it to use c#'s stopwatch for shorter + more accurate measurement

vickycl · 2023-07-31T09:19:32Z

package/com.unity.formats.usd/Tests/Editor/EditorAnalyticsBaseFixture.cs

+        public UsdAnalyticsEvent usd;
+    }
+
+    public class SharedAnalyticsEvent


The shared event is no longer used anywhere- can it be removed now?

In which case can we remove a layer of abstraction and use the UsdAnalyticsEvent directly instead of the AnalyticsEvent wrapper? To simplify lines like var exportUsdEventMsg = ((UsdAnalyticsEventExport)exportEvent.usd).msg; a bit, as they're hard to parse

for eg var exportUsdEventMsg = ((UsdAnalyticsEventExport)exportEvent).eventData; in that case would be easier to reason at a glance what it's getting

vickycl · 2023-07-31T09:21:20Z

package/com.unity.formats.usd/Tests/Editor/EditorExportAnalyticsTests.cs

+            Assert.IsNotNull(exportEvent);
+            var exportUsdEventMsg = ((UsdAnalyticsEventExport)exportEvent.usd).msg;
+
+            Assert.IsTrue(exportUsdEventMsg.Succeeded, "Expected True for re-import Successful");


think this should be 'export successful'

vickycl · 2023-07-31T09:21:46Z

package/com.unity.formats.usd/Tests/Editor/EditorExportAnalyticsTests.cs

+            Assert.IsNotNull(exportEvent);
+            var exportUsdEventMsg = ((UsdAnalyticsEventExport)exportEvent.usd).msg;
+
+            Assert.IsTrue(exportUsdEventMsg.Succeeded, "Expected True for re-import Successful");


As above :)

vickycl · 2023-07-31T09:22:36Z

package/com.unity.formats.usd/Tests/Editor/EditorExportAnalyticsTests.cs

+            Assert.IsNotNull(exportEvent);
+            var exportUsdEventMsg = ((UsdAnalyticsEventExport)exportEvent.usd).msg;
+
+            Assert.IsFalse(exportUsdEventMsg.Succeeded, "Expected Fail for re-import Successful");


As above :)

vickycl · 2023-07-31T09:29:01Z

package/com.unity.formats.usd/Tests/Editor/EditorAnalyticsBaseFixture.cs

+
+    public class UsdAnalyticsEvent
+    {
+        public string type;


Am I understanding correctly that this actually stores the event name string eg editor.USDFileImport, or is it actually the C# type name of the event? If it is the former I'd suggest a more explicit variable name :)

nono its the former, its the event name, i'll change it to something like "name" or "eventName"

actually i cant change this as the result json's variable is called "type" and as far as i know Unity's JsonUtility does not have something similar to JsonProperty

vickycl · 2023-07-31T11:01:34Z

package/com.unity.formats.usd/Tests/Editor/EditorAnalyticsBaseFixture.cs

+
+    public class SharedAnalyticsEvent
+    {
+        // check if the class for this is in Unity source (if its a bit convoluted just make it)


What does this comment mean?

It can be removed, but i meant to check if theres a class with the same structure as "SharedAnalyticsEvent" already defined within Unity, and if so, use that instead of creating it here

lee-aandrew added 2 commits June 28, 2023 18:09

Adds Analytics Auto Test

4cf765c

Adds Analytics Testcases

bbe7752

lee-aandrew requested review from vickycl and a team July 10, 2023 01:55

Fixes formatting issues

f5baed0

Removes dependency on Newtonsoft Json + updates requirement for Unity…

756fbb4

… version to be 2023.2

SimonBoorer approved these changes Jul 10, 2023

View reviewed changes

michaeljblain requested changes Jul 10, 2023

View reviewed changes

lee-aandrew added 3 commits July 10, 2023 17:38

Attempt at adding AddPackage at the beginning of the test

c327942

Remove 2 testing lines

b92a095

Removes the AddPackage.cs and dependency on the analytics debugger fr…

678ac6f

…om the manifest.json | Adds analytics to the formats.usd package

lee-aandrew added 10 commits July 26, 2023 16:39

Adds Analytics Auto Test

27e2dbc

Adds Analytics Testcases

021fec9

Fixes formatting issues

079bc20

Removes dependency on Newtonsoft Json + updates requirement for Unity…

d5018a8

… version to be 2023.2

Attempt at adding AddPackage at the beginning of the test

f8b2a18

Remove 2 testing lines

00f3942

Removes the AddPackage.cs and dependency on the analytics debugger fr…

aa6bc05

…om the manifest.json | Adds analytics to the formats.usd package

Rebase with dev - Test Codes refactor

370d07c

Increase wait frame count for better stability

38c98f6

lee-aandrew changed the base branch from dev to add-test-export-data-structure-kept July 26, 2023 20:46

lee-aandrew changed the base branch from add-test-export-data-structure-kept to dev July 26, 2023 20:46

lee-aandrew added 3 commits July 26, 2023 17:07

Apply test code refactoring changes

d7ad1f1

Reverting unnecessary changes

4fa0878

Adds analytics test enabling argument

2e7398e

lee-aandrew commented Jul 27, 2023

View reviewed changes

Fix formatting for yamato script

2fba851

vickycl reviewed Jul 31, 2023

View reviewed changes

applying code review fixes

5908426

Unity-Technologies deleted a comment from lee-aandrew May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[USDU-407] Add automated tests for Analytics #398

[USDU-407] Add automated tests for Analytics #398

lee-aandrew commented Jul 10, 2023

SimonBoorer commented Jul 10, 2023

michaeljblain left a comment

michaeljblain Jul 10, 2023

lee-aandrew commented Jul 25, 2023 •

edited

Loading

lee-aandrew Jul 27, 2023

vickycl Jul 27, 2023

vickycl left a comment

vickycl Jul 11, 2023

vickycl Jul 31, 2023

vickycl Jul 27, 2023

vickycl Jul 27, 2023

lee-aandrew Aug 1, 2023

vickycl Jul 27, 2023

lee-aandrew Aug 1, 2023

vickycl Jul 27, 2023

lee-aandrew Aug 2, 2023

vickycl Jul 31, 2023

vickycl Jul 31, 2023

vickycl Jul 31, 2023

vickycl Jul 31, 2023

vickycl Jul 31, 2023

vickycl Jul 31, 2023

vickycl Jul 31, 2023

lee-aandrew Aug 1, 2023

lee-aandrew Aug 2, 2023

vickycl Jul 31, 2023

lee-aandrew Aug 1, 2023

[USDU-407] Add automated tests for Analytics #398

Are you sure you want to change the base?

[USDU-407] Add automated tests for Analytics #398

Conversation

lee-aandrew commented Jul 10, 2023

Purpose of this PR

Testing

Overall Product Risks

Additional information

SimonBoorer commented Jul 10, 2023

michaeljblain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lee-aandrew commented Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vickycl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lee-aandrew commented Jul 25, 2023 •

edited

Loading